[LLVMdev] anchoring explicit template instantiations

[Chris Lattner]

On Dec 1, 2011, at 1:13 PM, David Blaikie wrote:
>>> (there's also some legitimate unreachable code warnings I'd be happy
>>> to fix as I find them, things like:
>>> 
>>> --- a/lib/Support/CommandLine.cpp
>>> +++ b/lib/Support/CommandLine.cpp
>>> @@ -294,10 +294,7 @@ static inline bool ProvideOption(Option *Handler,
>>> StringRef ArgName,
>>>     break;
>>> 
>>>   default:
>>> -    errs() << ProgramName
>>> -         << ": Bad ValueMask flag! CommandLine usage error:"
>>> -         << Handler->getValueExpectedFlag() << "\n";
>>> -    llvm_unreachable(0);
>>> +    llvm_unreachable("Bad ValueMask flag!");
>> 
>> This patch would lose the expected flag value, which is unfortunate.
> 
> It is unfortunate - though I'm not sure whether you're suggesting that
> the change should be made anyway, or not.
> 
> I'd say the whole unreachable could be dropped & an assertion placed
> inside getValueExpectedFlag which is where the actual unsafe bit->enum
> conversion is occuring.

That makes sense to me.  If a new enum were ever added to that case, presumably we'd get a warning in the switch if it doesn't have a default case.

> (that would still leave the need for a default in this switch because
> one of the values of the enum isn't covered - ValueMask, but really
> that probably shouldn't be one of the enumeration values anyway,
> should it? (looks like it should just be a hidden constant somewhere
> in the implementation details of llvm::Option))

Why not add a case for ValueMask but not a default: ?

>>> (a return after an llvm_unreachable - muddied up with a bit of
>>> syntactic tidyup))
>> 
>> This is an abuse of llvm_unreachable.  It should just replace "if ule" check with:
> 
> Ah, so it is. I hadn't noticed (was just looking at the simple
> transformation without actually parsing the semantics any further).
> 
> Committed with assert instead of abusive unreachable in r145627

Thanks!

-Chris