[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:207
+  } else {
+    auto I = FileSpec.find_last_of("/\\");
+    I = (I == FileSpec.size()) ? 0 : (I + 1);
----------------
lattner wrote:
> Is it standard to drop the path prefix?  Is this what happens when sourcemgr emits errors and warning diagnostics?
No, it is not dropped in messages. However, it adds horrible clutter to the detailed record report, since the paths can be long. I convinced myself that there will rarely, if ever, be a duplicate filename and so the source locations will be clear.

(Check the examples above and picture them with much longer filenames.)


================
Comment at: llvm/lib/Support/SourceMgr.cpp:210
+    return FileSpec.substr(I).str() + ":" +
+           std::to_string(FindLineNumber(Loc, BufferID));
+  }
----------------
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.


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

https://reviews.llvm.org/D88600