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

[George Rimar via Phabricator via llvm-commits]

grimar added inline comments.


================
Comment at: ELF/ScriptLexer.h:45
   size_t Pos = 0;
+  std::string Context;
 
----------------
peter.smith wrote:
> 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. 
May be, or I was thinking about following names now:
`ErrorContext`, `setErrorContext`, `dropErrorContext`


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

I tried 2 more ways here:
* Simple class that sets/drops context in constructor/destructor.
* Storing `std::weak_ptr<std::string> Context`. That way we can do something like:

```
std::shared_ptr<std::string> setErrorContext(const Twine& Msg) {
  auto Ret = make_shared<std::string>(Msg.cstr());
  Context = Ret;
  return Ret;
}
...
auto C = setErrorContext("blah blah blah");
<and while this shared pointer is alive, following code will print context>
...

if (auto C = Context.lock())
  error(*C);

```

But I am not sure it worth to do now, when there is only single place where context is used.

> Would it be worth using "section definition for output section" rather than "description for output section" to match the > Linker Script manual?

I'll change it, thanks. My wording was based on method name here.


https://reviews.llvm.org/D37013