[PATCH] D44105: TableGen: add !isa operation

[Nicolai Hähnle via Phabricator via llvm-commits]

nhaehnle added inline comments.


================
Comment at: docs/TableGen/LangIntro.rst:213-216
+    Note that this does not consider the possibility of casting between
+    records/objects and strings. That is, a string 'foo' is-not-a 'Bar', even
+    if a record named 'foo' with superclass 'Bar' exists.
+
----------------
tra wrote:
> I've found this confusing. Why would anyone expect a string argument to !isa<>() to be magically cast to a record? I would after this comment, though. :-) IMO the first line of the description alone is sufficient.
Done.


================
Comment at: lib/TableGen/Record.cpp:1235-1238
+  using Key = std::pair<RecTy *, Init *>;
+  static DenseMap<Key, IsAOpInit *> ThePool;
+
+  Key TheKey(std::make_pair(CheckType, Expr));
----------------
tra wrote:
> Why not use FoldingSet we use everywhere else?
> 
> I can't tell whether we're in danger of pointer reuse here, though if we are we'd need both `CheckType` and `Expr` to be freed, reallocated and passed to this function simultaneously in order to trigger a problem. Still...
> 
Changed to use FoldingSet.

Yes, the pointer reuse thing is indeed a theoretical problem, but it's pretty pervasive for all objects that can directly or indirectly reference a `Record`. This should really be cleaned up at some point.


================
Comment at: lib/TableGen/TGParser.cpp:958
+    if (Lex.getCode() != tgtok::r_paren) {
+      TokError("expected ')' in unary operator");
+      return nullptr;
----------------
tra wrote:
> Did you mean "in `!isa` operator" ?
The dangers of copy & paste :)

Fixed.


Repository:
  rL LLVM

https://reviews.llvm.org/D44105