[PATCH] D102277: [llvm-objcopy][NFC] Refactor CopyConfig structure - categorize options.

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

Something is a bit odd with the diff.



================
Comment at: llvm/tools/llvm-objcopy/ConfigManager.cpp:556-557
+  // Parse lazy options.
+  if (!AreELFLazyOptionsParsed) {
+    AreELFLazyOptionsParsed = true;
 
----------------
I really don't like this boolean here. We have Optional for a reason, and it does exactly this without the need of the additional boolean, right?

My gut says that our "unsupported option" handling needs redoing. Some of the options are applicable for multiple formats, but not yet implemented. Such options belong in the CommonConfig, which means we can cleanly reference them from within the file-format specific configs. I don't think we should be warning for other options. E.g. if a user uses an option that is tied to ELF, such as the new symbol visibility option, because the concept is not applicable in other formats, it shouldn't be checked for. This then means we can avoid referencing file format specific configs from each other, which I think is a good design.

What do you think?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102277