[PATCH] D27900: [ELF] - Keep the source file/line location information separate from the object file location information.

[Rui Ueyama via Phabricator via llvm-commits]

ruiu added a comment.

Overall, I think you designed this a bit too much. I don't want to add that many small wrapper functions for the existing error/warn/fatal functions.



================
Comment at: ELF/Error.cpp:84
 
+void elf::hint(const Twine &Msg) {
+  std::lock_guard<std::mutex> Lock(Mu);
----------------
If you add `hint` after `warn` in the header, please add to the same place in .cpp.


================
Comment at: ELF/Error.h:40
 void warn(const Twine &Msg);
 
+void hint(const Twine &Msg);
----------------
Remove this blank line.


================
Comment at: ELF/InputSection.cpp:42
 template <class ELFT>
+void elf::errorAtOffset(const InputSectionBase<ELFT> &IS,
+                        const llvm::Twine &Msg, typename ELFT::uint Off) {
----------------
I don't think this is a good addition because we already have too many error() wrapper functions. Aggregating two function calls into one doesn't help us that much, and I think it actually decreases readability. Please write it in two functions.


================
Comment at: ELF/InputSection.cpp:225
 template <class ELFT>
-std::string InputSectionBase<ELFT>::getLocation(typename ELFT::uint Offset) {
+static std::string getExtendedName(const InputSectionBase<ELFT> *IS,
+                                   typename ELFT::uint Offset) {
----------------
This is another `Ext` and that should be avoided.


================
Comment at: ELF/InputSection.h:147
+  // error message.
+  std::string getLocationExt(uintX_t Offset) const;
 
----------------
Please avoid `Ext` suffix because it doesn't convey meaning (with that naming scheme we'll end up having something like getDataExtExt.) You need to name variables/functions based on what they are instead of whether they are new or old.


================
Comment at: ELF/SymbolTable.cpp:372
   print(NewLoc + ": duplicate symbol '" + toString(*Existing) + "'");
-  print(OldLoc + ": previous definition was here");
+  std::string NewLocHint = D->Section->getLocationExt(D->Value);
+  hint("definition found in " + NewLocHint);
----------------
Why do you need this, `OldLoc` and `OldLocHint`? You can just write in place.


================
Comment at: ELF/SymbolTable.cpp:376
+  std::string OldLoc = toString(D->Section->getFile());
+  hint(OldLoc + ": previous definition was here");
+  std::string OldLocHint = ErrSec->getLocationExt(ErrOffset);
----------------
This results in an odd message.

  note: foo.o: previous definition was here

This should be

  note: previous definition was here: foo.o



================
Comment at: ELF/Target.cpp:58
 
+static void errorAtLocation(const uint8_t *Loc, const llvm::Twine &E) {
+  std::pair<std::string, std::string> Msg = getErrorLocation(Loc);
----------------
Please avoid writing a new wrapper.


================
Comment at: ELF/Target.cpp:64
+
+static void fatalAtLocation(const uint8_t *Loc, const llvm::Twine &E) {
+  errorAtLocation(Loc, E);
----------------
Please avoid writing a new wrapper function.

What is this function? I think we don't want to print out "fatal error encountered" at end.


https://reviews.llvm.org/D27900