[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/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");
+
+    // Add a location for the class definition.
+    CurRec->appendLoc(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

 
 #define DEBUG_TYPE "ctags-emitter"
 
 namespace {
 
 class Tag {
 private:
   const std::string *Id;
-  SMLoc Loc;
+  ArrayRef<SMLoc> Locs;
+
 public:
-  Tag(const std::string &Name, const SMLoc Location)
-      : Id(&Name), Loc(Location) {}
+  Tag(const std::string &Name, ArrayRef<SMLoc> Locs) : Id(&Name), Locs(Locs) {}
   int operator<(const Tag &B) const { return *Id < *B.Id; }
   void emit(raw_ostream &OS) const {
+    for (auto Loc : Locs) {
       const MemoryBuffer *CurMB =
           SrcMgr.getMemoryBuffer(SrcMgr.FindBufferContainingLoc(Loc));
       auto BufferName = CurMB->getBufferIdentifier();
-    std::pair<unsigned, unsigned> LineAndColumn = SrcMgr.getLineAndColumn(Loc);
+      std::pair<unsigned, unsigned> LineAndColumn =
+          SrcMgr.getLineAndColumn(Loc);
       OS << *Id << "\t" << BufferName << "\t" << LineAndColumn.first << "\n";
     }
+  }
 };
 
 class CTagsEmitter {
 private:
   RecordKeeper &Records;
 public:
   CTagsEmitter(RecordKeeper &R) : Records(R) {}
 
   void run(raw_ostream &OS);
-
-private:
-  static SMLoc locate(const Record *R);
 };
 
 } // End anonymous namespace.
 
-SMLoc CTagsEmitter::locate(const Record *R) {
-  ArrayRef<SMLoc> Locs = R->getLoc();
-  return !Locs.empty() ? Locs.front() : SMLoc();
-}
-
 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)
-    Tags.push_back(Tag(C.first, locate(C.second.get())));
+    Tags.push_back(Tag(C.first, C.second->getLoc()));
   for (const auto &D : Defs)
-    Tags.push_back(Tag(D.first, locate(D.second.get())));
+    Tags.push_back(Tag(D.first, D.second->getLoc()));
   // 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.