[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));
+  }
----------------
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.


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

https://reviews.llvm.org/D88600