[PATCH] Fixes and features for Consumed analysis

[Christian Wailes]

With regards to the tests, those already exist.  They are in the
warn-consumed-parsing.cpp file.  As for the switch statements, I'll move
those to fully-covered switches.

I was not claiming that keeping patches limited is a bad idea, or was of
limited benefit in general.  Yes, I should have submitted this as multiple
patches, but to go back now and dig the different parts out of the commit
out would be time consuming, for very little benefit *in this case*.  All
of the things touched are inside my experimental code, or are the removal
of simple asserts.  I'd really like to focus on producing actual work, and
keep this as a lesson for next time.

- Chris

On Wed, Aug 21, 2013 at 9:19 AM, Reid Kleckner <rnk at google.com> wrote:

> On Wed, Aug 21, 2013 at 6:54 AM, Aaron Ballman <aaron at aaronballman.com>wrote:
>
>> On Tue, Aug 20, 2013 at 7:16 PM, Christian Wailes
>> <chriswailes at google.com> wrote:
>> >> > +
>> >> > +  if (!FunDecl->hasAttr<CallableWhenUnconsumedAttr>()) return;
>> >> > +
>> >> > +  if (PState.IsVar) {
>> >> > +    const VarDecl *Var = PState.getVar();
>> >> > +
>> >> > +    switch (StateMap->getState(Var)) {
>> >> > +    case CS_Consumed:
>> >> > +      Analyzer.WarningsHandler.warnUseWhileConsumed(
>> >> > +        FunDecl->getNameAsString(), Var->getNameAsString(),
>> >> > +        Call->getExprLoc());
>> >> > +      break;
>> >> > +
>> >> > +    case CS_Unknown:
>> >> > +      Analyzer.WarningsHandler.warnUseInUnknownState(
>> >> > +        FunDecl->getNameAsString(), Var->getNameAsString(),
>> >> > +        Call->getExprLoc());
>> >> > +      break;
>> >> > +
>> >> > +    default:
>> >> > +      break;
>> >>
>> >> Should the default be unreachable?
>> >>
>> >
>> > No, there are other cases besides Consumed and Unknown.  I just don't
>> want
>> > to do anything for those cases.
>>
>> I worry that this will cause extra warnings in some of the build bots
>> because the other enumerants are not covered in the switch statement.
>> The same is true elsewhere.
>>
>
> It is a bit of a clang coding convention to use fully-covered switches
> with no default.  That way, when someone adds an enum value, the compiler
> will warn on all of the switches missing cases.
>
> We shouldn't get warnings for a partially covered switch with a default
> (like this one), although we will for fully covered switches with a default.
>
> There are only two enums vs one default, so I'd cover the switch to
> leverage the warning.
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20130821/7ab96d24/attachment.html>