[PATCH] D88600: [TableGen] New backend to print fully detailed records

[Paul C. Anagnostopoulos via Phabricator via llvm-commits]

Paul-C-Anagnostopoulos added inline comments.


================
Comment at: llvm/lib/Support/SourceMgr.cpp:210
+    return FileSpec.substr(I).str() + ":" +
+           std::to_string(FindLineNumber(Loc, BufferID));
+  }
----------------
lattner wrote:
> Paul-C-Anagnostopoulos wrote:
> > Paul-C-Anagnostopoulos wrote:
> > > lattner wrote:
> > > > Shouldn't this also include column numbers?
> > > > 
> > > > Making this utility accessible to other clients of SourceMgr is nice, but if so, please change SourceMgr itself to use it for its internal formatting of the location information.  This ensures that things stay consistent.
> > > While testing the new backend, I found the column numbers to be of no use. The primary purpose of showing the source locations is to help people figure out the path that a record or field took through all the TableGen definitions. Knowing the column number of, say, the value of a field is no more useful than knowing the line number of its definition.
> > > 
> > > I will investigate using this new function in the SourceMgr itself.
> > The formatting of messages is already spread between a SourceMgr function and an SMDiagnostic function. The latter is the most general one, but it writes directly to a stream rather than returning a string. So I don't think the new function is contributing much to the confusion. I will, however, add a comment about this.
> > 
> > One solution is to write an SMDiagnostic low-level message formatter that returns a string, then use it everywhere. However, that requires you to create an SMDiagnostic instance just to format a location.  So perhaps the work should be done by a new SMLoc function. The point is that there are SMLoc formatting requirements that have nothing to do with diagnostic messages.
> > 
> > I have added this issue to my list of things to investigate.
> Ok, no problem.  This just means that this method isn't a general "SourceMgr::getFormattedLocation" method, it is something specific to the tblgen backend.  Please move it there, and the concern is addressed, thanks!
It would be general if I add a second optional argument to include the column number. Then it can format a source location in the full style, or in various abbreviated styles.

I put the function in SourceMgr so that the new backend wouldn't directly call such functions as FindBufferContaining and GetBufferInfo. Let me check something . . . 

So, only two existing TableGen backends call those functions directly: CTagsEmitter and DAGIselMatcherEmitter. So I propose to (1) create this new function we're discussing; (2) use it in those two backends; (3) then address the idea of one general-purpose location formatter.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D88600/new/

https://reviews.llvm.org/D88600