[TableGen] Add a location for a class definition that was forward-declared

[rusyaev-roman]

Summary

This change improves ctags generation for tablegen files.

For the following example

class A;

class A {
  int a;
}

Previously, tags were generated only for a forward declaration of class 'A'.

This patch allows generating tags for the forward declarations
and further definition of class 'A'.

[rusyaev-roman]

changed commit message

[rusyaev-roman]

Removed unsed 'private' access specifier.

[rusyaev-roman]

Rebased

[rusyaev-roman]

This failure is not related to my changes

[rusyaev-roman]

Rebase

[barannikov88]

LGTM, but I can't merge either.

[rusyaev-roman]

@silvas @nhaehnle @sunfish Could you take a look at this patch and then merge it please? I have no permission.

[nhaehnle]

Thanks for the patch. Could you please add a test case?

[rusyaev-roman]

In D129935#3664884, @nhaehnle wrote:

Thanks for the patch. Could you please add a test case?

I've already added the testcase. You can take a look at llvm/test/TableGen/GenTags.td

[nhaehnle]

Right you are, sorry for my confusion. The patch isn't quite right because it overrides the current meaning of the Locs array (which is to track multiclass instantiations), and that can lead to confusion e.g. with PrintMessage. I took the liberty of making some adjustments before committing the patch (394a388d140dc9e74178532501cddb558a589398)

[rusyaev-roman]

In D129935#3665649, @nhaehnle wrote:

Right you are, sorry for my confusion. The patch isn't quite right because it overrides the current meaning of the Locs array (which is to track multiclass instantiations), and that can lead to confusion e.g. with PrintMessage. I took the liberty of making some adjustments before committing the patch (394a388d140dc9e74178532501cddb558a589398)

I think the original changes cannot lead to confusion in this case. The behavior for multi-class instantiations has not been affected. The original patch has improved tracking for classes, because before these changes, if we have a declaration (or empty definition) and a definition for some class, we can obtain a location only for its forward-declaration. But the original changes provided a location for the definition as well. What do you think?

[nhaehnle]

In D129935#3665997, @rusyaev-roman wrote:

In D129935#3665649, @nhaehnle wrote:

Right you are, sorry for my confusion. The patch isn't quite right because it overrides the current meaning of the Locs array (which is to track multiclass instantiations), and that can lead to confusion e.g. with PrintMessage. I took the liberty of making some adjustments before committing the patch (394a388d140dc9e74178532501cddb558a589398)

I think the original changes cannot lead to confusion in this case. The behavior for multi-class instantiations has not been affected. The original patch has improved tracking for classes, because before these changes, if we have a declaration (or empty definition) and a definition for some class, we can obtain a location only for its forward-declaration. But the original changes provided a location for the definition as well. What do you think?

My concern here is with the behavior of PrintMessage and friends. The original change would produce misleading output there. WIth the current change, PrintMessage will print the location of the definition (if it is known).

[rusyaev-roman]

In D129935#3668112, @nhaehnle wrote:

My concern here is with the behavior of PrintMessage and friends. The original change would produce misleading output there. WIth the current change, PrintMessage will print the location of the definition (if it is known).

I see what you mean and it's reasonable. But I think that it's better to hold all locations for the record and decide what to report at a user side. Maybe I'll prepare a related patch in the future, because I feel that introducing 'forward declaration' field is redundant in this case.

Also, I'm going to improve ctags support for tablegen a little bit more in the future.

Anyway, thanks for merging and some suggestions!

llvm/include/llvm/TableGen/Record.h

AssertionInfo(SMLoc Loc, Init *Condition, Init *Message)
         : Loc(Loc), Condition(Condition), Message(Message) {}
   };
 
 private:
   Init *Name;
   // Location where record was instantiated, followed by the location of
   // multiclass prototypes used.
   SmallVector<SMLoc, 4> Locs;
+  SmallVector<SMLoc, 0> ForwardDeclarationLocs;
   SmallVector<Init *, 0> TemplateArgs;
   SmallVector<RecordVal, 0> Values;
   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;
 
const std::string getNameInitAsString() const {
     return getNameInit()->getAsUnquotedString();
   }
 
   void setName(Init *Name);      // Also updates RecordKeeper.
 
   ArrayRef<SMLoc> getLoc() const { return Locs; }
   void appendLoc(SMLoc Loc) { Locs.push_back(Loc); }
 
+  ArrayRef<SMLoc> getForwardDeclarationLocs() const {
+    return ForwardDeclarationLocs;
+  }
+
+  // Update a class location when encountering a (re-)definition.
+  void updateClassLoc(SMLoc Loc);
+
   // Make the type that this record should have based on its superclasses.
   RecordRecTy *getType();
 
   /// get the corresponding DefInit.
   DefInit *getDefInit();
 
   bool isClass() const { return IsClass; }
 

llvm/lib/TableGen/Record.cpp

void RecordVal::print(raw_ostream &OS, bool PrintSem) const {
   OS << getPrintType() << " " << getNameInitAsString();
 
   if (getValue())
     OS << " = " << *getValue();
 
   if (PrintSem) OS << ";\n";
 }
 
+void Record::updateClassLoc(SMLoc Loc) {
+  assert(Locs.size() == 1);
+  ForwardDeclarationLocs.push_back(Locs.front());
+
+  Locs.clear();
+  Locs.push_back(Loc);
+}
+
 void Record::checkName() {
   // Ensure the record name has string type.
   const TypedInit *TypedName = cast<const TypedInit>(Name);
   if (!isa<StringRecTy>(TypedName->getType()))
     PrintFatalError(getLoc(), Twine("Record name '") + Name->getAsString() +
                                   "' is not a string!");
 }
 

llvm/lib/TableGen/TGParser.cpp

bool TGParser::ParseClass() {
   Record *CurRec = Records.getClass(Lex.getCurStrVal());
   if (CurRec) {
     // If the body was previously defined, this is an error.
     if (!CurRec->getValues().empty() ||
         !CurRec->getSuperClasses().empty() ||
         !CurRec->getTemplateArgs().empty())
       return TokError("Class '" + CurRec->getNameInitAsString() +
                       "' already defined");
+
+    CurRec->updateClassLoc(Lex.getLoc());
   } else {
     // If this is the first reference to this class, create and add it.
     auto NewRec =
         std::make_unique<Record>(Lex.getCurStrVal(), Lex.getLoc(), Records,
                                   /*Class=*/true);
     CurRec = NewRec.get();
     Records.addClass(std::move(NewRec));
   }

llvm/test/TableGen/GenTags.td

+// RUN: llvm-tblgen --gen-ctags %s | FileCheck %s -DFILE=%s
+
+// CHECK: A [[FILE]] [[@LINE+1]]
+class A;
+
+// CHECK: A [[FILE]] [[@LINE+1]]
+class A {
+  string name = "A";
+}

llvm/utils/TableGen/CTagsEmitter.cpp

 using namespace llvm;
 
 #define DEBUG_TYPE "ctags-emitter"
 
 namespace {
 
 class Tag {
 private:
-  const std::string *Id;
-  SMLoc Loc;
+  StringRef Id;
+  StringRef BufferIdentifier;
+  unsigned Line;
 public:
-  Tag(const std::string &Name, const SMLoc Location)
-      : Id(&Name), Loc(Location) {}
-  int operator<(const Tag &B) const { return *Id < *B.Id; }
-  void emit(raw_ostream &OS) const {
+  Tag(StringRef Name, const SMLoc Location) : Id(Name) {
     const MemoryBuffer *CurMB =
-        SrcMgr.getMemoryBuffer(SrcMgr.FindBufferContainingLoc(Loc));
-    auto BufferName = CurMB->getBufferIdentifier();
-    std::pair<unsigned, unsigned> LineAndColumn = SrcMgr.getLineAndColumn(Loc);
-    OS << *Id << "\t" << BufferName << "\t" << LineAndColumn.first << "\n";
+        SrcMgr.getMemoryBuffer(SrcMgr.FindBufferContainingLoc(Location));
+    BufferIdentifier = CurMB->getBufferIdentifier();
+    auto LineAndColumn = SrcMgr.getLineAndColumn(Location);
+    Line = LineAndColumn.first;
+  }
+  int operator<(const Tag &B) const {
+    return std::make_tuple(Id, BufferIdentifier, Line) < std::make_tuple(B.Id, B.BufferIdentifier, B.Line);
+  }
+  void emit(raw_ostream &OS) const {
+    OS << Id << "\t" << BufferIdentifier << "\t" << Line << "\n";
   }
 };
 
 class CTagsEmitter {
 private:
   RecordKeeper &Records;
 public:
   CTagsEmitter(RecordKeeper &R) : Records(R) {}

 }
 
 void CTagsEmitter::run(raw_ostream &OS) {
   const auto &Classes = Records.getClasses();
   const auto &Defs = Records.getDefs();
   std::vector<Tag> Tags;
   // Collect tags.
   Tags.reserve(Classes.size() + Defs.size());
-  for (const auto &C : Classes)
+  for (const auto &C : Classes) {
     Tags.push_back(Tag(C.first, locate(C.second.get())));
+    for (SMLoc FwdLoc : C.second->getForwardDeclarationLocs())
+      Tags.push_back(Tag(C.first, FwdLoc));
+  }
   for (const auto &D : Defs)
     Tags.push_back(Tag(D.first, locate(D.second.get())));
   // Emit tags.
   llvm::sort(Tags);
   OS << "!_TAG_FILE_FORMAT\t1\t/original ctags format/\n";
   OS << "!_TAG_FILE_SORTED\t1\t/0=unsorted, 1=sorted, 2=foldcase/\n";
   for (const Tag &T : Tags)
     T.emit(OS);
 }
 
 namespace llvm {
 
 void EmitCTags(RecordKeeper &RK, raw_ostream &OS) { CTagsEmitter(RK).run(OS); }
 
 } // End llvm namespace.