[PATCH] D37013: [ELF] - Add additional information about location when emiting linkerscript's parsing errors.
George Rimar via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Tue Aug 22 08:07:37 PDT 2017
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
More information about the llvm-commits
mailing list