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

[Alexey Lapshin via Phabricator via llvm-commits]

avl added inline comments.


================
Comment at: llvm/tools/llvm-objcopy/ConfigManager.cpp:556-557
+  // Parse lazy options.
+  if (!AreELFLazyOptionsParsed) {
+    AreELFLazyOptionsParsed = true;
 
----------------
jhenderson wrote:
> 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?
> 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?
> 
Let`s temporarily delay this point.

> 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?

My understanding is: whether such change is OK or not depends on the usage idea of the llvm-objcopy tool(and friends llvm-strip...). Is it supposed to work with multiple formats at a time? 

I think, the intention of such checks : 


```
  if (MachO.StripSwiftSymbols || MachO.KeepUndefined)
    return createStringError(llvm::errc::invalid_argument,
                             "option not supported by llvm-objcopy for ELF");
```

is to avoid multi-format usage: 


```
llvm-strip -S -T ELF-file MachO-file
llvm-strip: error: option not supported by llvm-objcopy for ELF
```

If we will avoid format-specific checks then we would allow multi-format usages. 

Is it OK?

If we would like to preserve the idea of avoiding multi-format usages and we are fine with redoing "unsupported options" handling then it might be better to implement that idea explicitly(if I did not miss something):

1. Before starting to parse options, detect the format of the input file(single source file, first source file, first file inside an archive).
2. Parse options for the detected format and display warning if there were specified options for another format (other than we detected at step 1.).
3. while handling source file(s), ignore(or fail?) files which have a format different from what we detected at stage 1.
4. while handling source file(s), check options from CommonConfig: If there are some unimplemented yet - then display the warning message.

Such an approach, explicitly force only single format usage(which looks like a good thing). Also, it avoids the necessity to implement lazy options, which complicate interfaces and implementation.

What do you think?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102277