Page MenuHomePhabricator

Add format_dynamic_key_arg attribute to improve "-Wformat" warnings for functions that load the formatting string dynamically based on a key value
ClosedPublic

Authored by erik.pilkington on Nov 28 2016, 7:59 AM.

Details

Summary

This patch adds a new attribute called format_dynamic_key_arg. It's intended to be used instead of format_arg for methods/functions that load the formatting string from some external source based on the given key value. The attribute tells Clang that it has to avoid -Wformat warnings when key strings have no formatting specifiers. Otherwise it tells Clang that it can assume that the key strings use the same formatting layout as the external formatting string values (i.e. Clang can perform the normal -Wformat related checks).

This attribute is useful for certain Objective-C methods like NSBundle.localizedStringForKey that load the values dynamically based on a specified key value.

Diff Detail

Repository
rL LLVM

Event Timeline

arphaman updated this revision to Diff 79404.Nov 28 2016, 7:59 AM
arphaman retitled this revision from to Add format_dynamic_key_arg attribute to improve "-Wformat" warnings for functions that load the formatting string dynamically based on a key value.
arphaman updated this object.
arphaman added reviewers: aaron.ballman, manmanren.
arphaman set the repository for this revision to rL LLVM.
arphaman updated this object.
arphaman updated this object.
arphaman added a subscriber: cfe-commits.
aaron.ballman added inline comments.Dec 16 2016, 8:03 AM
include/clang/Basic/Attr.td
862 ↗(On Diff #79404)

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?

lib/Sema/SemaDeclAttr.cpp
5632 ↗(On Diff #79404)

We ask that all of these handle*Attr() functions have the same signature, so adding extra parameters isn't something we do. At some point, we want to generate more boilerplate code from the attribute definition files, and so having different parameter lists makes that much harder to accomplish. A better approach would be to make a function called handleFormatDynamicKeyArg() that calls a helper function with the extra argument, and modify handleFormatArgAttr() to call that helper function as well.

arphaman updated this revision to Diff 81950.Dec 19 2016, 7:52 AM
arphaman marked an inline comment as done.

The updated patch renames the attribute to loads_format_specifier_value_using_key and addresses Aaron's comments

include/clang/Basic/Attr.td
862 ↗(On Diff #79404)

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.

arphaman updated this revision to Diff 81952.Dec 19 2016, 8:02 AM

Update to fix a test failure.

aaron.ballman added inline comments.Dec 19 2016, 12:51 PM
include/clang/Basic/Attr.td
862 ↗(On Diff #79404)

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.

include/clang/Basic/AttrDocs.td
1759 ↗(On Diff #81952)

value -> specifier (same below).

1766 ↗(On Diff #81952)

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).

lib/Sema/SemaChecking.cpp
4408 ↗(On Diff #81952)

I think the original formatting was easier to read.

4418 ↗(On Diff #81952)

Same here.

4593 ↗(On Diff #81952)

Can use const auto * here.

6303 ↗(On Diff #81952)

The original formatting was better.

test/Sema/attr-format_arg.c
29 ↗(On Diff #81952)

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++.

erik.pilkington commandeered this revision.Aug 13 2019, 2:39 PM
erik.pilkington added a reviewer: arphaman.
erik.pilkington added a subscriber: erik.pilkington.

Stealing this (with Alex's permission). I think it makes more sense to add a special case for the method -[NSBundle localizedStringForKey:value:table:] rather then add a new general purpose attribute that'll only be used in one place.

Herald added a project: Restricted Project. · View Herald TranscriptAug 13 2019, 2:39 PM

Special case localizedStringForKey instead of using an attribute.

erik.pilkington edited reviewers, added: rjmccall; removed: manmanren.Aug 13 2019, 2:41 PM

I agree with just special-casing this for now.

clang/lib/Sema/SemaChecking.cpp
6800 ↗(On Diff #214925)

It's more efficient to do these checks with isStr instead of looking up an IdentifierInfo. You can add a Selector::isKeywordSelector(ArrayRef<StringRef>) method to make that equally convenient for selectors.

Address review comments.

rjmccall accepted this revision.Aug 13 2019, 4:54 PM

One minor request (which will be dead code in your patch), but otherwise LGTM.

clang/include/clang/Basic/IdentifierTable.h
754 ↗(On Diff #214977)

Thanks. Could you go ahead and add isUnarySelector(StringRef) for consistency?

This revision is now accepted and ready to land.Aug 13 2019, 4:54 PM
aaron.ballman accepted this revision.Aug 14 2019, 8:36 AM

I like this approach much better! LGTM, aside from a minor nit.

clang/include/clang/Basic/IdentifierTable.h
754 ↗(On Diff #214977)

Also, the function should be marked const, I would imagine.

This revision was automatically updated to reflect the committed changes.