[PATCH] D32195: Object: Remove ModuleSummaryIndexObjectFile class.

[Peter Collingbourne via Phabricator via llvm-commits]

pcc added inline comments.


================
Comment at: clang/lib/CodeGen/BackendUtil.cpp:63
 
+static llvm::cl::opt<bool> IgnoreEmptyThinLTOIndexFile(
+    "ignore-empty-index-file", llvm::cl::ZeroOrMore,
----------------
pcc wrote:
> tejohnson wrote:
> > pcc wrote:
> > > tejohnson wrote:
> > > > Not sure llvm options are encouraged in clang (I only see a couple of cases currently).
> > > > 
> > > > Can you instead just move llvm::getModuleSummaryIndexForFile into BitcodeReader and avoid a lot of these changes?
> > > The other clients do not need to be able to handle a null index (and I think some of them were crashing if they got one). I want to remove the possibility of the other clients getting a null index from this function.
> > > 
> > > The llvm::cl option is a pre-existing evil, this change isolates it to the only place that needs it and makes it easier to change to a proper clang option later.
> > I only see one llvm::cl::opt that is linked into clang itself (the rest are used by various tools under clang/tools AFAICT. And I suspect that one snuck in - I believe I tried to add another one at some point and it was called out in a review, but I can't seem to find that now. 
> > 
> > Since the internal option is off by default, I don't understand the issue about other clients crashing if they got a null index. Presumably they crashed because it was null, which isn't expected without enabling the internal option.
> > 
> > Also, by moving llvm::getModuleSummaryIndexForFile into BitcodeReader you will avoid some of the churn/duplication in llvm (calls to getFileOrSTDIN). 
> I agree that it is bad to have a cl::opt in clang, but what is even worse is sneaking one into clang by putting it in llvm instead, as it makes it harder to understand that the option is in fact a clang option. I think it was you who added this option, so I would ask you to remove it eventually, but I don't think it should block this change.
> 
> Regarding null, yes, the clients would crash if the internal option is enabled, which is why it shouldn't even be possible to enable it.
> 
> Regarding duplication, clients already have to open regular bitcode module files in order to read them, so I don't think it is a big deal.
I think this is the review where you originally tried to add the option to clang: D28362

I think the right way to support this option would be to add a -cc1 flag to the frontend. If you like, I could add a FIXME to do that.


https://reviews.llvm.org/D32195