On Thu, Dec 1, 2011 at 9:11 AM, Chris Lattner <clattner at apple.com> wrote:> > On Dec 1, 2011, at 12:08 AM, David Blaikie wrote: > >> On Wed, Nov 30, 2011 at 10:42 PM, Chris Lattner <clattner at apple.com> wrote: >>> On Nov 29, 2011, at 12:26 AM, David Blaikie wrote: >>>> For a bit of an experiment I've been trying to compile LLVM & Clang >>>> with -Weverything (disabling any errors that seem like more noise/less >>>> interesting). One warning I've recently hit a few instances of is >>>> -Wweak-vtable which is, in fact, an explicitly documented LLVM coding >>>> standard ( http://llvm.org/docs/CodingStandards.html#ll_virtual_anch >>>> ). Some instances of this have been easy to fix (see attached patch - >>>> if it looks good to anyone I'll check that in) but a particular set of >>>> them have been a little more problematic. >>> >>> Nice, please commit your patch. I don't know about explicit instantiations though, maybe a C++ guru on the clang list knows. >> >> Thanks Chris, committed as r145578. I don't suppose you'll mind some >> similar commits as I encounter them? > > Yep, please feel free.Thanks - I'll see what comes up.>> (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 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))>> +++ b/lib/Support/APInt.cpp >> @@ -1440,16 +1440,14 @@ APInt APInt::sqrt() const { >> APInt nextSquare((x_old + 1) * (x_old +1)); >> if (this->ult(square)) >> return x_old; >> + if (this->ule(nextSquare)) { >> APInt midpoint((nextSquare - square).udiv(two)); >> APInt offset(*this - square); >> if (offset.ult(midpoint)) >> return x_old; >> + } >> + llvm_unreachable("Error in APInt::sqrt computation"); >> } >> >> (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, - David
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 r145627Thanks! -Chris
>> (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: ?An unreachable case for ValueMask? Yeah, that'd be better than default. Not exposing the mask values in the enums at all seems nicer, though - so far as I can tell they're an implementation detail of llvm::Option. - see the attached patch. (a bit of reworking inside Option flags to avoid the need for masks entirely (though it does make the size of the bitmasks fields "magic" based on the number of elements in each enum) - but it makes the flags simpler so that they don't each have to use bits beyond the previous one) So I can do that, or just keep it the way it is & make a ValueMask case with unreachable in it. - David -------------- next part -------------- A non-text attachment was scrubbed... Name: option_masks.diff Type: text/x-diff Size: 17732 bytes Desc: not available URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20111201/754e4344/attachment.diff>
Possibly Parallel Threads
- [LLVMdev] anchoring explicit template instantiations
- [LLVMdev] anchoring explicit template instantiations
- [LLVMdev] anchoring explicit template instantiations
- [LLVMdev] anchoring explicit template instantiations
- [LLVMdev] anchoring explicit template instantiations