[PATCH] D110577: [llvm-cxxfilt] Enable support for D programming language demangling

[Luís Ferreira via Phabricator via llvm-commits]

ljmf00 added inline comments.


================
Comment at: llvm/tools/llvm-cxxfilt/llvm-cxxfilt.cpp:79-99
   if (Types ||
       ((DecoratedLength >= 2 && strncmp(DecoratedStr, "_Z", 2) == 0) ||
        (DecoratedLength >= 4 && strncmp(DecoratedStr, "___Z", 4) == 0)))
     Undecorated = itaniumDemangle(DecoratedStr, nullptr, nullptr, &Status);
 
   if (!Undecorated &&
       (DecoratedLength > 6 && strncmp(DecoratedStr, "__imp_", 6) == 0)) {
----------------
dblaikie wrote:
> dblaikie wrote:
> > Perhaps this code could be refactored to be shared with llvm-symbolizer? (eg: the code being updated in D110664 here: `llvm/lib/DebugInfo/Symbolize/Symbolize.cpp`)
> Maybe the refactoring would be better done in D110664 and then that could be the basis for this patch to make one change that'd apply to both llvm-cxxfilt and llvm-symbolizer/symbolizing library.
I can wait for that refactor on https://reviews.llvm.org/D110664 and make a patch to symbolize code


================
Comment at: llvm/tools/llvm-cxxfilt/llvm-cxxfilt.cpp:21
 #include <cstdlib>
+#include <cstring>
 #include <iostream>
----------------
dblaikie wrote:
> jhenderson wrote:
> > ljmf00 wrote:
> > > jhenderson wrote:
> > > > I don't think you need to include this header - `strncmp` is already used in this file, so it's clearly being included through the existing include hierarchy. See also https://llvm.org/docs/CodingStandards.html#include-as-little-as-possible.
> > > Yeah, my editor LSP plugin probably added it automatically by mistake, and no apparent duplication was detected. I'm going to remove it!
> > No problem!
> Generally we should include anything we depend on - not rely on indirect inclusions. (the style guide gives some allowance for this - like if some other header just has to have provided a definition/inclusion of a thing, that's fine - but in this case there's no strong connection between whatever header includes something for strncmp so it's suitable to include it here, so if that other header is refactored to remove the need/use of the header it doesn't break this code)
> 
> So this #include is probably good/proper here.
> 
> The advice in the style guide is about not including unused headers and not including headers when a forward declaration would suffice.
I can agree on that, I just also agreed with @jhenderson more likely because `itanium` and `rust` code parts are also using `strncmp` without such include.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D110577/new/

https://reviews.llvm.org/D110577