aaron.ballman added inline comments.
================
Comment at: include/clang/Basic/Attr.td:862
+def FormatDynamicKeyArg : InheritableAttr {
+ let Spellings = [GCC<"format_dynamic_key_arg">];
+ let Args = [IntArgument<"FormatIdx">];
----------------
arphaman wrote:
> aaron.ballman wrote:
> > Does GCC support this attribute as well? If not, this should use the GNU
> > spelling instead of the GCC one. Also, should this have a C++11 spelling in
> > the clang namespace?
> >
> > The name doesn't really convey much about what the attribute is doing,
> > mostly because it doesn't seem obvious what "key" means.
> >
> > It seems that the crux of what this attribute says is that the attributed
> > function accepts a string argument of a format specifier and returns the
> > same format specifier. Perhaps `returns_format_specifier` is a better name?
> > Does GCC support this attribute as well? If not, this should use the GNU
> > spelling instead of the GCC one.
>
> No, it doesn't. Thanks for pointing that out, I fixed it.
>
> > Also, should this have a C++11 spelling in the clang namespace?
>
> I don't know, is that required for new attributes? I don't need the C++11
> spelling personally, and I don't know if there is anyone else who's
> interested in this attribute.
>
> > The name doesn't really convey much about what the attribute is doing,
> > mostly because it doesn't seem obvious what "key" means.
> >
> > It seems that the crux of what this attribute says is that the attributed
> > function accepts a string argument of a format specifier and returns the
> > same format specifier. Perhaps returns_format_specifier is a better name?
>
> You're right, the name should convey the intended meaning better. I switched
> to `loads_format_specifier_value_using_key`, how does that sound?
> I think it sounds better than just `returns_format_specifier` because I would
> like to see this attribute used only for a particular kind of function. This
> kind of function should load the value at runtime using the key from a
> platform-specific file/database that might also be accessible at
> compile-time. It shouldn't be used for functions that might just modify the
> given key, which, IMO, `returns_format_specifier` can imply.
>> Also, should this have a C++11 spelling in the clang namespace?
> I don't know, is that required for new attributes? I don't need the C++11
> spelling personally, and I don't know if there is anyone else who's
> interested in this attribute.
It's not required, but not everyone likes GNU-style attributes, so having
something a bit less awful is a kindness.
>> The name doesn't really convey much about what the attribute is doing,
>> mostly because it doesn't seem obvious what "key" means.
>> It seems that the crux of what this attribute says is that the attributed
>> function accepts a string argument of a format specifier and returns the
>> same format specifier. Perhaps returns_format_specifier is a better name?
>You're right, the name should convey the intended meaning better. I switched
>to loads_format_specifier_value_using_key, how does that sound?
> I think it sounds better than just returns_format_specifier because I would
> like to see this attribute used only for a particular kind of function. This
> kind of function should load the value at runtime using the key from a
> platform-specific file/database that might also be accessible at
> compile-time. It shouldn't be used for functions that might just modify the
> given key, which, IMO, returns_format_specifier can imply.
It's a bit wordy, but I think it's better than the original name.
================
Comment at: include/clang/Basic/AttrDocs.td:1759
+function accepts a key string that corresponds to some external ``printf`` or
+``scanf``-like format string value, loads the value that corresponds to the
+given key and returns that value.
----------------
value -> specifier (same below).
================
Comment at: include/clang/Basic/AttrDocs.td:1766
+specifiers found in the key string.
+ }];
+}
----------------
Given that this is uses novel terminology like "key string", I think an example
would be useful to add.
The docs should also mention that this attribute accepts an argument, and that
the argument is 1-based instead of 0-based (I can't count how many times that's
tripped me up).
================
Comment at: lib/Sema/SemaChecking.cpp:4408
+static void CheckFormatString(
+ Sema &S, const FormatStringLiteral *FExpr, const Expr *OrigFormatExpr,
+ ArrayRef<const Expr *> Args, bool HasVAListArg, unsigned format_idx,
----------------
I think the original formatting was easier to read.
================
Comment at: lib/Sema/SemaChecking.cpp:4418
// True string literals are then checked by CheckFormatString.
-static StringLiteralCheckType
-checkFormatStringExpr(Sema &S, const Expr *E, ArrayRef<const Expr *> Args,
- bool HasVAListArg, unsigned format_idx,
- unsigned firstDataArg, Sema::FormatStringType Type,
- Sema::VariadicCallType CallType, bool InFunctionCall,
- llvm::SmallBitVector &CheckedVarArgs,
- UncoveredArgHandler &UncoveredArg,
- llvm::APSInt Offset) {
- tryAgain:
+static StringLiteralCheckType checkFormatStringExpr(
+ Sema &S, const Expr *E, ArrayRef<const Expr *> Args, bool HasVAListArg,
----------------
Same here.
================
Comment at: lib/Sema/SemaChecking.cpp:4593
+ unsigned ArgIndex = FKA->getFormatIdx();
+ if (const CXXMethodDecl *MD = dyn_cast<CXXMethodDecl>(ND))
+ if (MD->isInstance())
----------------
Can use `const auto *` here.
================
Comment at: lib/Sema/SemaChecking.cpp:6303
+static void CheckFormatString(
+ Sema &S, const FormatStringLiteral *FExpr, const Expr *OrigFormatExpr,
+ ArrayRef<const Expr *> Args, bool HasVAListArg, unsigned format_idx,
----------------
The original formatting was better.
================
Comment at: test/Sema/attr-format_arg.c:29
+const char *formatKeyArgError2(const char *format)
+ __attribute__((loads_format_specifier_value_using_key(0))); //
expected-error{{'loads_format_specifier_value_using_key' attribute parameter 1
is out of bounds}}
----------------
You should also add tests for:
it diagnoses if the attribute is added to something other than a
function/method.
it diagnoses if the attribute has no arguments/arguments of the wrong type.
it works properly on a class member function in C++.
Repository:
rL LLVM
https://reviews.llvm.org/D27165
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
