[PATCH] Refactor attribute arguments

[Aaron Ballman]

My testing showed it to be the case -- when using the index returned
from checkFunctionOrMethodArgumentIndex, it was always off by one or
two depending on context.  Specifically:

const char* g2(const char*) __attribute__((format_arg(2)));

checkFunctionOrMethodIndex would return index 0 because it cares about
the const char * -- adding 1 to it when passing in the argument to
addAttr (to make it one based instead of zero based) was not
sufficient; I would get crashes in other parts of the system for that
test case.  Having it pass in two is consistent with the previous
behavior.

~Aaron

On Mon, Jul 22, 2013 at 5:30 PM, Eli Friedman <eli.friedman at gmail.com> wrote:
> On Mon, Jul 22, 2013 at 11:13 AM, Aaron Ballman <aaron at aaronballman.com> wrote:
>> And then I promptly attached the wrong patch.  Sorry.  Correct patch
>> attached here.
>>
>> ~Aaron
>>
>> On Mon, Jul 22, 2013 at 2:09 PM, Aaron Ballman <aaron at aaronballman.com> wrote:
>>> There was already a helper method named
>>> checkFunctionOrMethodArgumentIndex whcih would perform semantic
>>> checking for attributes.  This patch refactors alloc_size, nonnull,
>>> format_args and the ownership attributes to use the helper method
>>> instead of the copy and pasted code.
>>>
>>> No functional changes intended, but there are some new test cases to
>>> test coverage.
>>>
>>> ~Aaron
>
> @@ -3082,8 +2964,14 @@
>      return;
>    }
>
> +  // We cannot use the ArgIdx returned from checkFunctionOrMethodArgumentIndex
> +  // because that has corrected for the implicit this parameter, and is zero-
> +  // based.  The attribute expects what the user wrote explicitly.
> +  llvm::APSInt Val;
> +  IdxExpr->EvaluateAsInt(Val, S.Context);
> +
>    D->addAttr(::new (S.Context)
> -             FormatArgAttr(Attr.getRange(), S.Context, Idx.getZExtValue(),
> +             FormatArgAttr(Attr.getRange(), S.Context, Val.getZExtValue(),
>                             Attr.getAttributeSpellingListIndex()));
>  }
>
> Are you sure?  We have tests in test/SemaCXX/attr-format.cpp which
> seem to say this is wrong.
>
> -Eli