Document cyclic dependency.

[hzeller]

To be fixed later, but this way it is already documented. (what needs to be done is probably break out the libraries in the BUILD file better).

[github-actions]

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

[github-actions]

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

[maliberty]

It would be helpful to state the actual symbols involved in the circular dependency.

[hzeller]

I mentioned one in one of the files (sta::Cell), but there were a few more that all required dbSta.hh (currently off computer, but can have a look later)

[maliberty]

You can get that from sta/include/sta/NetworkClass.hh

[hzeller]

Ok, looked
both need sta::Cell and sta::Instance

NetwokClass.hh only has a forward declare, not a definition.

[maliberty]

There is no definition - it is just used for type puning.

[hzeller]

yes, but even opaque types should be defined (like what you do in dbSta.hh; and the comment even gives a reason why this is needed for the RTTI constructs).

[maliberty]

As it states there

// talking to the OpenSTA author about implementing these
// upstream instead.

is the right fix. Putting it in dbSta was a hack and causes the cycles you see. @QuantamHD what came of those discussions? We can put it in our fork if necessary.

[hzeller]

This might be one of the cases where a Macro can improve readability because it describes the intent.
I am thinking of something like this:

#define MAKE_OPAQUE_TYPE(T)                     \
class T                                         \
{                                               \
 public:                                        \
  T() = delete;                                 \
}

MAKE_OPAQUE_TYPE(Cell);
MAKE_OPAQUE_TYPE(Instance);
// ...
#undef  MAKE_OPAQUE_TYPE

Since it will not only improve the use of the types in RTTI context, but also strictly improve the readability, it might be easier to upstream such change to sta/NetworkClass.hh

[hzeller]

Converting this to draft, as other includes should be used; but leaving it open so that we don't forget to move the opaque type stuff into sta.

[maliberty]

I think the easiest way to break the cycle is to just move these defintions into their own header and make it a more base level dep than dbSta which is more high level.

[hzeller]

w.r.t. #8407 (comment)

The change would be something like

--- a/include/sta/NetworkClass.hh
+++ b/include/sta/NetworkClass.hh
@@ -34,19 +34,29 @@
 
 namespace sta {
 
+#define MAKE_OPAQUE_TYPE(T)                     \
+class T                                         \
+{                                               \
+ public:                                        \
+  T() = delete;                                 \
+}
+
+MAKE_OPAQUE_TYPE(Cell);
+MAKE_OPAQUE_TYPE(Library);
+MAKE_OPAQUE_TYPE(Port);
+MAKE_OPAQUE_TYPE(Instance);
+MAKE_OPAQUE_TYPE(Pin);
+MAKE_OPAQUE_TYPE(Term);
+MAKE_OPAQUE_TYPE(Net);
+MAKE_OPAQUE_TYPE(ViewType);
+
+#undef  MAKE_OPAQUE_TYPE
+
 class Network;
 class NetworkEdit;
 class NetworkReader;
-class Library;
-class Cell;
-class Port;
 class PortDirection;
-class Instance;
-class Pin;
-class Term;
-class Net;
 class ConstantPinIterator;
-class ViewType;
 class LibertyLibrary;
 
 typedef Iterator<Library*> LibraryIterator;

should I file a PR against https://github.com/The-OpenROAD-Project/OpenSta or try to parallaxsw ?

[maliberty]

I have no idea if you can get it into parallax but let's see. I would include a description of the motivating issue with RTTI.