[PATCH] D67039: [ELF] Add a spell corrector for "undefined symbol" diagnostics

[Fangrui Song via Phabricator via llvm-commits]

MaskRay added inline comments.


================
Comment at: ELF/Relocations.cpp:721
+  StringRef name = sym.getName();
+  for (size_t i = 0, e = name.size(); i != e + 1; ++i) {
+    // Insert a character before name[i].
----------------
grimar wrote:
> ruiu wrote:
> > IIUC, this function costs (N+1)*74 where N is the length of a symbol name. Symbol names can be very long. Doesn't this too slow?
> It's enabled for the first two undefined symbols. Even with large N, like 100 it shouldn't probably be too slow for the error reporting case?
I think the complexity is fine. I avoid making the size of symtab a factor in the time complexity. Doing mutation in the input is probably fine.

In a large executable, I get a long mangled name of N=4398 bytes.
O(74*N) is not too bad.


================
Comment at: ELF/Relocations.cpp:727
+      if (suggest(newName))
+        break;
+    }
----------------
grimar wrote:
> Seems it would be simpler to make `suggest` return `const Symbol *` (on success) or `nullptr` (on fail)?
> You should be able to early return nicely then:
> 
> ```
> if (const Symbol *S = suggest(newName))
>   return S;
> ```
> 
> and avoid doing things like
> 
> ```
>     if (corrected)
>       break;
> ```
> 
> 
Thanks. This simplification is available since this is now a new function.


================
Comment at: ELF/Relocations.cpp:752
+static void reportUndefinedSymbol(const UndefinedDiag &undef,
+                                  bool spellCorrector) {
   Symbol &sym = *undef.sym;
----------------
peter.smith wrote:
> I think you mean to use spellCorrector as a verb (do spelling correction), but it reads like a noun (spellCorrector is some object that does spelling correction). I suggest doSpellingCorrection or correctSpelling.
Will use `correctSpelling`


================
Comment at: ELF/Relocations.cpp:823
+  size_t i = 0;
+  for (const UndefinedDiag &undef : undefs)
+    if (!undef.locs.empty()) {
----------------
grimar wrote:
> It feels `for` would look a bit better?
> 
> ```
> for (size_t I = 0; I<undefs.size(); ++I)
>   if (!undefs[I].locs.empty())
>     reportUndefinedSymbol<ELFT>(undefs[I], i < 2);
> ```
I'll use

`for (auto it : enumerate(undefs))`



Repository:
  rLLD LLVM Linker

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

https://reviews.llvm.org/D67039