[PATCH] D62275: [llvm-objdump] Add warning if --disassemble-functions specifies an unknown symbol

[James Henderson via Phabricator via llvm-commits]

jhenderson added reviewers: rupprecht, grimar, MaskRay.
jhenderson added a comment.

I like what you're doing, and I think having count and insert operations on a StringSet make a lot of sense from a somewhat naive perspective, but I think those should be in a separate change, with unit tests and with reviewers who know that class better than I do. You can cite this review in that one for the motivation (also citing that it makes it more STL-like).

I've added a few more reviewers, partly because I'm away all of next week.



================
Comment at: llvm/test/tools/llvm-objdump/X86/warn-missing-disasm-func.test:1
+## Warn if --disassemble-functions specifies an unknown symbol
+# RUN: yaml2obj %s | llvm-objdump - --disassemble-functions=foo 2>&1 | FileCheck %s
----------------
Nit: missing trailing full stop.


================
Comment at: llvm/test/tools/llvm-objdump/X86/warn-missing-disasm-func.test:10
+  Machine: EM_X86_64
+...
+
----------------
By the way, the trailing "..." isn't needed in any case I've seen so far for yaml2obj.


================
Comment at: llvm/test/tools/llvm-objdump/X86/warn-missing-disasm-func.test:12
+
+# CHECK: warning: Failed to disassemble missing function foo.
----------------
I think the more common approach is for warnings and errors to start with lower case and not have a trailing full stop.


================
Comment at: llvm/tools/llvm-objdump/llvm-objdump.cpp:379
 
+void warn(const Twine &Message) {
+  SmallVector<char, 256> MessageVec;
----------------
`StringRef` would be more appropriate here, I think.


================
Comment at: llvm/tools/llvm-objdump/llvm-objdump.cpp:1412-1413
   }
+  auto MissingDisasmFuncsSet =
+      set_difference(DisasmFuncsSet, FoundDisasmFuncsSet);
+  if (!MissingDisasmFuncsSet.empty())
----------------
I think it would be clearer here if you don't use `auto`.


================
Comment at: llvm/tools/llvm-objdump/llvm-objdump.cpp:1415
+  if (!MissingDisasmFuncsSet.empty())
+    for (const StringRef &MissingDisasmFunc : MissingDisasmFuncsSet.keys())
+      warn("Failed to disassemble missing function " + MissingDisasmFunc);
----------------
No need for the `const &` for a `StringRef`.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D62275