[PATCH] D27165: Add format_dynamic_key_arg attribute to improve "-Wformat" warnings for functions that load the formatting string dynamically based on a key value

[Aaron Ballman via Phabricator via cfe-commits]

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