[PATCH] D52104: lld-link: print demangled symbol names for "undefined symbol" diagnostics

[Zachary Turner via Phabricator via llvm-commits]

zturner added inline comments.


================
Comment at: llvm/lib/Demangle/MicrosoftDemangle.cpp:1020-1022
+  if (initializeOutputStream(nullptr, nullptr, OS, 1024))
+    // FIXME: Propagate out-of-memory as an error?
+    std::terminate();
----------------
erik.pilkington wrote:
> thakis wrote:
> > zturner wrote:
> > > Is this returning `true` if it fails?  Gross.  Can we make it return `false` if it fails?
> > Returning true on error is reasonably common in the codebase: http://llvm-cs.pcc.me.uk/?q=true+on+error (e.g. https://reviews.llvm.org/D45898#inline-454138) I too wish it wasn't so.
> > 
> > (I'm also calling an existing function here, so I'd prefer to not change all the existing callers.)
> I think LLVM style is true on error, aka unix style. I've seen a lot of both though. I don't see any mention in the coding style guide, but I think this would be a really good thing to have there. Maybe we should start a bikeshed on llvm-dev.
FWIW I think we should fix all of these, and in the past we've fixed instances of it on the grounds that it's bad form.  The case of `DiagnoseUseOfDecl` is slightly more defensible because the presence of `Diagnose` sort of hints that it the function returns true, something is wrong.  (I'd still change it or rename the function though).  That said, it's true you are calling an existing function, but previously that function was only called 6 times, now it is called 11 times.  I would prefer not to propagate this confusing usage pattern because it makes the code demonstrably worse imo.  


https://reviews.llvm.org/D52104