[LLVMdev] anchoring explicit template instantiations
clattner at apple.com
Thu Dec 1 15:03:04 PST 2011
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,
>>> - 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
More information about the llvm-dev