[PATCH] D32195: Object: Remove ModuleSummaryIndexObjectFile class.

[Teresa Johnson via Phabricator via llvm-commits]

tejohnson 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:
> > 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). 


https://reviews.llvm.org/D32195