[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

[Alex Lorenz via Phabricator via cfe-commits]

arphaman 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">];
----------------
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.


Repository:
  rL LLVM

https://reviews.llvm.org/D27165