[PATCH] D37013: [ELF] - Add additional information about location when emiting linkerscript's parsing errors.

[Peter Smith via Phabricator via llvm-commits]

peter.smith added a comment.

I think that this is definitely an improvement, I've made some suggestions but I haven't got a strong opinion.



================
Comment at: ELF/ScriptLexer.h:45
   size_t Pos = 0;
+  std::string Context;
 
----------------
It might be better to call this LocationContext if it is specifically going to be used within setLocationContext() and dropLocationContex(). This might help prevent it being used/overwritten in other non LocationContext situations by mistake. 


================
Comment at: ELF/ScriptParser.cpp:607
 OutputSection *ScriptParser::readOutputSectionDescription(StringRef OutSec) {
+  setLocationContext("unable to parse description for output section '" +
+                     OutSec + "':");
----------------
Is it worth an RAII wrapper so that you don't have to explicitly remember to call dropLocationContext()?


================
Comment at: ELF/ScriptParser.cpp:607
 OutputSection *ScriptParser::readOutputSectionDescription(StringRef OutSec) {
+  setLocationContext("unable to parse description for output section '" +
+                     OutSec + "':");
----------------
peter.smith wrote:
> Is it worth an RAII wrapper so that you don't have to explicitly remember to call dropLocationContext()?
Would it be worth using "section definition for output section" rather than "description for output section" to match the Linker Script manual?


https://reviews.llvm.org/D37013