-
Notifications
You must be signed in to change notification settings - Fork 14.4k
[IR] "modular-format" attribute for functions using format strings #147429
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Draft
mysterymath
wants to merge
2
commits into
users/mysterymath/modular-printf/reloc.none
Choose a base branch
from
users/mysterymath/modular-printf/ir
base: users/mysterymath/modular-printf/reloc.none
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+79
−0
Draft
Changes from all commits
Commits
Show all changes
2 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this intended to be specific to printf style format strings? Or will it potentially be reusable for other formatting languages? The text here just says "format string" without saying what kind. For futureproofing, perhaps there should be some kind of a type parameter here saying which formatting mechanism is referred to? That way if two incompatible formatting syntaxes need to be supported later, there's space for expansion.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a good point and a good idea. There's already a format string type present in the
format
attribute, so it seems ideal to directly reuse that, especially since the other information is already copied from that attribute, for the same reason. Tying the two would mean that adding support for a new format string type would require adding it in both places, but having a no-op format string type in clang'sformat
implementation doesn't seem like it would be that big of a deal.It was also momentally tempting to combine this format string type with the impl_name field, but on further thought, this would require that an executable have only one implementation for each format string type. That may commonly be the case, but I can't think of an intrinsic reason it would be so, and keeping them separate would only cost a few extra bytes per annotated call, so it doesn't seem worth it to combine them.
I'll make the change throughout the PR stack and report back.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks.
An afterthought: even printf and scanf are different enough that they'd need separate handling, so I don't even need to postulate a hypothetical format string syntax from some other language's standard library. For example, your check for arguments of actual floating-point type is fine for telling that a printf call doesn't need float formatting, but for a scanf call, you'd need to look for pointers to floating types instead.