[PATCH] D69650: [ELF] Suggest an arbitrary C++ overload as an alternative spelling

[David Blaikie via Phabricator via llvm-commits]

dblaikie added inline comments.


================
Comment at: lld/ELF/Relocations.cpp:705-706
+  unsigned len;
+  return def.consume_front("_Z") && !def.consumeInteger(10, len) &&
+         len == ref.size() && def.take_front(len) == ref;
+}
----------------
I'm not super comfortable with this hand-written demangling (maintenance burden/code duplication, etc) - is there something in the itanium demangling library that already does this/can be reused (or refactored so it can be reused)?


================
Comment at: lld/ELF/Relocations.cpp:791-794
+    symtab->forEachSymbol([&](Symbol *sym) {
+      if (canSuggestExternCXX(name, sym->getName()))
+        s = sym;
+    });
----------------
If we're only going to make one suggestion, is there any way to short-circuit the forEachSymbol to stop on the first candidate found, rather than searching for a lot of candidates that'll be ignored?

(equally, should we be printing more candidates, and maybe printing the source line/file they were defined on in the error message to help people find the right one?)


================
Comment at: lld/test/ELF/undef-suggest-extern-cxx.s:15
+# CHECK-NEXT: >>> referenced by {{.*}}
+# CHECK-NEXT: >>> did you mean: one of the C++ overloads such as foo(int)
+
----------------
This situation might be worth a few more words for the user.

This current phrasing makes it sounds like the person who wrote the call to foo, might've intended to call foo(int) - when they might've actually called foo(int), but with C mangling & there's a good chance the /call/ is correct (it'd be rare that someone wrote a header file intending to only use it for C++ and manage to just by accident write perfectly valid C in that header file without the intent that that file ever be included from C code) - and that it's the definition in C++ that needs to be fixed.

"did you mean to define foo(int) (or some other overload) as extern "C"?"

might be the phrasing to go for?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69650