More strongly formulate the using namespace coding practice.

[hzeller]

No description provided.

[github-actions]

clang-tidy review says "All clean, LGTM! 👍"

[gemini-code-assist]

Code Review

This pull request strengthens the coding practice regarding using namespace by changing 'Avoid' to 'Never' and providing more detailed guidance on the acceptable use of using namespace::symbol;. The change is clear and improves the coding guidelines. I've found a small typo in the updated text.

[github-actions]

clang-tidy review says "All clean, LGTM! 👍"

[povik]

I disagree with this as a universal requirement. The rsz code is closely tied to sta and I feel strongly the developers of rsz are better served with a blanket using namespace sta; in resizer .cc files. I prefer dealing with potential conflicts to the churn of updating the using statements. I think we have a clang-tidy warning on an unused import which is easy to run into on any rsz change.

[maliberty]

This argument could apply to sta and dbSta or est. It could apply to odb and any number of modules. As an experiment I converted SizeUpMove.cc to no using-declarations and found it readable with a clear sense of origin. (Not to mention many unused statements).

How are developers "better served" would help to understand your viewpoint. Are they saving typing? Visual clutter? Other?

[povik]

I'm discussing using namespace sta; vs individual using sta::foo; and I would like to save on the number of times I need to revisit code in response to feedback from the compiler or clang-tidy, which are prompting me to add or remove an import.

This is in addition to a large number of checks any openroad PR needs to pass through (e.g. warningless compilation under a specific compiler, Tcl linter, clang-format, metric checks of various kinds, regression tests which are interdependent between modules so e.g. a rsz change causes updates in cts). I think it's worth trying to reduce the burden if some part of it can be relaxed.

[maliberty]

I tend to avoid that by just not relying on using-declarations for simple types. The compiler simply checks it. Many modules have minimal using declarations. rsz is rather heavy in this regard.

The interdependent regressions are just a bad idea and that should be fixed.

[povik]

rsz is rather heavy in this regard.

I'm not sure SizeUpMove.cc is representative. rsz code can get very verbose without using of the sta types.

The interdependent regressions are just a bad idea and that should be fixed.

I agree and try to give a hand fixing it.

[povik]

For what it's worth the function below would repeat sta:: twelve times. I think I would be ok with importing none of sta, or importing all of it through using namespace. It's the middle ground where we only import some identifiers which can lead to a lot of back and forth with the compiler and clang-tidy.

void RepairDesign::checkDriverArcSlew(const sta::Scene* corner,
                                      const sta::Instance* inst,
                                      const sta::TimingArc* arc,
                                      float load_cap,
                                      float limit,
                                      float& violation)
{
  const auto dcalc_ap = corner->dcalcAnalysisPtIndex(max_);
  const sta::RiseFall* in_rf = arc->fromEdge()->asRiseFall();
  sta::GateTimingModel* model = dynamic_cast<sta::GateTimingModel*>(arc->model());
  sta::Pin* in_pin = network_->findPin(inst, arc->from()->name());

  if (model && in_pin) {
    const bool use_ideal_clk_slew
        = arc->set()->role()->genericRole() == sta::TimingRole::regClkToQ()
          && sta_->cmdMode()->clkNetwork()->isIdealClock(in_pin);
    sta::Slew in_slew
        = use_ideal_clk_slew
              ? sta_->cmdMode()->clkNetwork()->idealClkSlew(in_pin, in_rf, max_)
              : graph_->slew(graph_->pinLoadVertex(in_pin), in_rf, dcalc_ap);
    const sta::Pvt* pvt = corner->sdc()->pvt(inst, max_);
    if (pvt == nullptr) {
      pvt = corner->sdc()->operatingConditions(max_);
    }

    sta::ArcDelay arc_delay;
    sta::Slew arc_slew;
    model->gateDelay(pvt, in_slew, load_cap, false, arc_delay, arc_slew);

    if (arc_slew > limit) {
      violation = max(arc_slew - limit, violation);
    }
  }
}

[maliberty]

I would opt for none rather than all. I agree a middle ground can be confusing.

Side note: I wouldn't repeat the type in
sta::GateTimingModel* model = dynamic_cast<sta::GateTimingModel*>(arc->model());
but just use auto with a cast. Agrees with https://google.github.io/styleguide/cppguide.html#Type_deduction "Local variable type deduction"

[maliberty]

Traditionally the alternative has been prefixes like odb uses db on the front of all its types. I think namespaces are better even if a few extra characters.

(In a previous system every name had a three-letter prefix and the first letter was always 'c'. It lead to some strange naming contortions).

[hzeller]

Code is written once but read many times.

While using namespace helps saving a few seconds during writing, it makes it much harder for the many readers of the code afterwards. Making it less ambiguous where a symbol is coming from is beneficial.

In the sta:: context in particular, there are some commonly sounding symbols such as INF, Vector or sort() that make the reader wonder where they are coming from for instance.

If the goal is to eventually reduce the strong interdependence, it is good to have it very visible just how many symbols are there merged into the current namespace. Spelling them out one by one makes that very obvious, while using namespace just hides it.
It also helps encouraging using the namespaces directly at symbols or reducing cognitive overhead by replacing repetitions like FooBarBazType *x = new FooBarBazType() with auto *x = new FooBarBazType().

The phrasing "avoid doing" sounds like it is a suggestion that can be ignored easily; while "never do" emphasizes that something is generally not an advisable idea. It's just a written coding practice, which doesn't mean that with good reasons it cannot be broken. But it will require more burden of explanation.