[TableGen] Change assertion information from a tuple to a struct [NFC]

[Paul-C-Anagnostopoulos]

Summary

This revision changes the representation of assertion information from a tuple to a struct. Assertion information is collected from 'assert' statements.

It makes the code easier to read.

[dblaikie]

llvm/include/llvm/TableGen/Record.h

1479

Could add a comment saying this ctor is here only so you can use make_unique (& I think C++20 will make this work without a user-defined/declared ctor like this (when we eventually adopt C++20, which is a long way off))

Alternatively, since there's only one make_unique, might be just as well to skip the ctor and have the construction code do this:

std::unique_ptr<AssertionInfo> x(new AssertionInfo{Loc, Condition, Message});

(maybe also with a comment there about why make_unique here until C++20 due to the lack of a ctor)

llvm/lib/TableGen/TGParser.cpp

368

This is some very strange/non-idiomatic code. Why are the member accesses qualified like this?

I'd expect this code to look more like: E.Assertion->Loc for instance

3218–3219

You can use 'auto' for the type here, since it's clear from the make_unique on the right what the type of this variable is.

Or you can roll it all into the addEntry call below like this:

addEntry(std::make_unique<Record::AssertionInfo>(ConditionLoc, Condition, Message));

[Paul-C-Anagnostopoulos]

Thanks, @dblaikie. I will incorporated your suggestions and the linties.

llvm/lib/TableGen/TGParser.cpp

368

I was hoping someone would tell me how to simplify this. Why do I need 'AssertionInfo::' this way, but not with '->Loc'?

[dblaikie]

(also a bunch of clang-format auto-comments on this review, please run this change through clang-format-patch or however else you like to do it to address those comments)

llvm/lib/TableGen/TGParser.cpp

368

I don't think you do? You could write it as (*E.Assertion).Loc as well. It's pretty uncommon to need to scope a member access like you have here (how'd you came across this syntax/end up writing it this way?) - /sometimes/ it may be useful to scope a member function access (x.Base::y()) to access a base member function rather than an override or shadowing in a derived class. (eg: if you want to override a function but delegate some functionality to the orgiinal base function, you could use Base::func() to do that delegation - but very rarely for member variable access)

[Paul-C-Anagnostopoulos]

llvm/lib/TableGen/TGParser.cpp

368

The (*E.Assertion) was just stupid. I did that because *E.Assertion was already there.

For the rest of it. I tried two or three variants until I got it. Let me check . . . nope, don't need the AssertionInfo::. No idea why I ended up with it, since I first assumed I didn't need it.

Twenty years of programming without worrying about pointers has taken its toll.

[Paul-C-Anagnostopoulos]

I have incorporated the lint changes and David's suggestions.

@dblaikie: Could you explain more about what I should say about the AssertionInfo constructor? I don't understand what you meant about the constructor and make_unique().

[dblaikie]

In D100854#2709200, @Paul-C-Anagnostopoulos wrote:

I have incorporated the lint changes and David's suggestions.

@dblaikie: Could you explain more about what I should say about the AssertionInfo constructor? I don't understand what you meant about the constructor and make_unique().

Oh, something like // user-defined dtor to support std::make_unique, can be removed in C++20 where braced init is supported

[craig.topper]

llvm/lib/TableGen/TGParser.cpp

437

Why std::move on the Condition and Message pointers? I don't think that's doing anything.

[Paul-C-Anagnostopoulos]

llvm/lib/TableGen/TGParser.cpp

437

Nope, it isn't. But it was when I slipped back into deleting-pointer-deletes-referent thinking mode. Another result of 20 years of programming in memory-managed languages.

[Paul-C-Anagnostopoulos]

Incorporate latest comments from @dblaikie and @craig.topper.

[dblaikie]

Seems reasonable

llvm/include/llvm/TableGen/Record.h

 
 inline raw_ostream &operator<<(raw_ostream &OS, const RecordVal &RV) {
   RV.print(OS << "  ");
   return OS;
 }
 
 class Record {
 public:
-  using AssertionTuple = std::tuple<SMLoc, Init *, Init *>;
+  struct AssertionInfo {
+    SMLoc Loc;
+    Init *Condition;
+    Init *Message;
+
+    AssertionInfo(SMLoc Loc, Init *Condition, Init *Message)
+        : Loc(Loc), Condition(Condition), Message(Message) {}
+  };
 
 private:
   static unsigned LastID;
 
   Init *Name;
   // Location where record was instantiated, followed by the location of
   // multiclass prototypes used.
   SmallVector<SMLoc, 4> Locs;
   SmallVector<Init *, 0> TemplateArgs;
   SmallVector<RecordVal, 0> Values;
-  // Vector of [source location, condition Init, message Init].
-  SmallVector<AssertionTuple, 0> Assertions;
+  SmallVector<AssertionInfo, 0> Assertions;
 
   // All superclasses in the inheritance forest in post-order (yes, it
   // must be a forest; diamond-shaped inheritance is not allowed).
   SmallVector<std::pair<Record *, SMRange>, 0> SuperClasses;
 
   // Tracks Record instances. Not owned by Record.
   RecordKeeper &TrackedRecords;
 
public:
   bool isClass() const { return IsClass; }
 
   ArrayRef<Init *> getTemplateArgs() const {
     return TemplateArgs;
   }
 
   ArrayRef<RecordVal> getValues() const { return Values; }
 
-  ArrayRef<AssertionTuple> getAssertions() const { return Assertions; }
+  ArrayRef<AssertionInfo> getAssertions() const { return Assertions; }
 
   ArrayRef<std::pair<Record *, SMRange>>  getSuperClasses() const {
     return SuperClasses;
   }
 
   /// Determine whether this record has the specified direct superclass.
   bool hasDirectSuperClass(const Record *SuperClass) const;
 
void removeValue(Init *Name) {
     llvm_unreachable("Cannot remove an entry that does not exist!");
   }
 
   void removeValue(StringRef Name) {
     removeValue(StringInit::get(Name));
   }
 
   void addAssertion(SMLoc Loc, Init *Condition, Init *Message) {
-    Assertions.push_back(std::make_tuple(Loc, Condition, Message));
+    Assertions.push_back(AssertionInfo(Loc, Condition, Message));
   }
 
   void appendAssertions(const Record *Rec) {
     Assertions.append(Rec->Assertions);
   }
 
   void checkRecordAssertions();
 

llvm/lib/TableGen/Record.cpp

if (Init *V = Value.getValue()) {
                 "' after resolving references: " + VR->getAsUnquotedString() +
                 "\n");
       }
     }
   }
 
   // Resolve the assertion expressions.
   for (auto &Assertion : Assertions) {
-    Init *Value = std::get<1>(Assertion)->resolveReferences(R);
-    std::get<1>(Assertion) = Value;
-    Value = std::get<2>(Assertion)->resolveReferences(R);
-    std::get<2>(Assertion) = Value;
+    Init *Value = Assertion.Condition->resolveReferences(R);
+    Assertion.Condition = Value;
+    Value = Assertion.Message->resolveReferences(R);
+    Assertion.Message = Value;
   }
 }
 
 void Record::resolveReferences(Init *NewName) {
   RecordResolver R(*this);
   R.setName(NewName);
   R.setFinal(true);
   resolveReferences(R);

 // and message, then call CheckAssert().
 // Note: The condition and message are probably already resolved,
 //       but resolving again allows calls before records are resolved.
 void Record::checkRecordAssertions() {
   RecordResolver R(*this);
   R.setFinal(true);
 
   for (auto Assertion : getAssertions()) {
-    Init *Condition = std::get<1>(Assertion)->resolveReferences(R);
-    Init *Message = std::get<2>(Assertion)->resolveReferences(R);
-    CheckAssert(std::get<0>(Assertion), Condition, Message);
+    Init *Condition = Assertion.Condition->resolveReferences(R);
+    Init *Message = Assertion.Message->resolveReferences(R);
+    CheckAssert(Assertion.Loc, Condition, Message);
   }
 }
 
 #if !defined(NDEBUG) || defined(LLVM_ENABLE_DUMP)
 LLVM_DUMP_METHOD void RecordKeeper::dump() const { errs() << *this; }
 #endif
 
 raw_ostream &llvm::operator<<(raw_ostream &OS, const RecordKeeper &RK) {

llvm/lib/TableGen/TGParser.h

struct LetRecord {
     Init *Value;
     SMLoc Loc;
     LetRecord(StringInit *N, ArrayRef<unsigned> B, Init *V, SMLoc L)
       : Name(N), Bits(B), Value(V), Loc(L) {
     }
   };
 
   /// RecordsEntry - Holds exactly one of a Record, ForeachLoop, or
-  /// assertion tuple.
+  /// AssertionInfo.
   struct RecordsEntry {
     std::unique_ptr<Record> Rec;
     std::unique_ptr<ForeachLoop> Loop;
-    std::unique_ptr<Record::AssertionTuple> Assertion;
+    std::unique_ptr<Record::AssertionInfo> Assertion;
 
     void dump() const;
 
     RecordsEntry() {}
     RecordsEntry(std::unique_ptr<Record> Rec) : Rec(std::move(Rec)) {}
     RecordsEntry(std::unique_ptr<ForeachLoop> Loop)
         : Loop(std::move(Loop)) {}
-    RecordsEntry(std::unique_ptr<Record::AssertionTuple> Assertion)
+    RecordsEntry(std::unique_ptr<Record::AssertionInfo> Assertion)
         : Assertion(std::move(Assertion)) {}
   };
 
   /// ForeachLoop - Record the iteration state associated with a for loop.
   /// This is used to instantiate items in the loop body.
   ///
   /// IterVar is allowed to be null, in which case no iteration variable is
   /// defined in the loop at all. (This happens when a ForeachLoop is
   /// constructed by desugaring an if statement.)

llvm/lib/TableGen/TGParser.cpp

bool TGParser::addEntry(RecordsEntry E) {
   // If we are parsing a multiclass, add it to the multiclass's entries.
   if (CurMultiClass) {
     CurMultiClass->Entries.push_back(std::move(E));
     return false;
   }
 
   // If it is an assertion, then it's a top-level one, so check it.
   if (E.Assertion) {
-    CheckAssert(std::get<0>(*E.Assertion), std::get<1>(*E.Assertion), 
-                std::get<2>(*E.Assertion));
+    CheckAssert(E.Assertion->Loc, E.Assertion->Condition, E.Assertion->Message);
     return false;
   }
 
   // It must be a record, so finish it off.
   return addDefOne(std::move(E.Rec));
 }
 
 /// Resolve the entries in \p Loop, going over inner loops recursively
bool TGParser::resolve(const std::vector<RecordsEntry> &Source,
   for (auto &E : Source) {
     if (E.Loop) {
       Error = resolve(*E.Loop, Substs, Final, Dest);
 
     } else if (E.Assertion) {
       MapResolver R;
       for (const auto &S : Substs)
         R.set(S.first, S.second);
-      Init *Condition = std::get<1>(*E.Assertion)->resolveReferences(R);
-      Init *Message = std::get<2>(*E.Assertion)->resolveReferences(R);
+      Init *Condition = E.Assertion->Condition->resolveReferences(R);
+      Init *Message = E.Assertion->Message->resolveReferences(R);
 
-      if (Dest) {
-        std::unique_ptr<Record::AssertionTuple> Tuple =
-            std::make_unique<Record::AssertionTuple>(std::get<0>(*E.Assertion),
+      if (Dest)
+        Dest->push_back(std::make_unique<Record::AssertionInfo>(E.Assertion->Loc,
                                                                 std::move(Condition),
-                                                     std::move(Message));
-        Dest->push_back(std::move(Tuple));
-      } else {
-        CheckAssert(std::get<0>(*E.Assertion), Condition, Message);
-      }
+                                                                std::move(Message)));
+      else
+        CheckAssert(E.Assertion->Loc, Condition, Message);
 
     } else {
       auto Rec = std::make_unique<Record>(*E.Rec);
       if (Loc)
         Rec->appendLoc(*Loc);
 
       MapResolver R(Rec.get());
       for (const auto &S : Substs)
bool TGParser::ParseAssert(MultiClass *CurMultiClass, Record *CurRec) {
 
   Init *Message = ParseValue(CurRec);
   if (!Message)
     return true;
 
   if (!consume(tgtok::semi))
     return TokError("expected ';'");
 
-  if (CurRec) {
+  if (CurRec)
     CurRec->addAssertion(ConditionLoc, Condition, Message);
-  } else {
-    std::unique_ptr<Record::AssertionTuple> Tuple =
-         std::make_unique<Record::AssertionTuple>(ConditionLoc, Condition, Message);
-    addEntry(std::move(Tuple));
-  }
- 
+  else
+    addEntry(std::make_unique<Record::AssertionInfo>(ConditionLoc, Condition,
+                                                     Message));
   return false;
 }
 
 /// ParseClass - Parse a tblgen class definition.
 ///
 ///   ClassInst ::= CLASS ID TemplateArgList? ObjectBody
 ///
 bool TGParser::ParseClass() {