-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Add pure/const function attribute (refs #12695) #6385
base: main
Are you sure you want to change the base?
Conversation
@@ -2360,6 +2360,7 @@ | |||
<!-- bool QFile::exists(const QString &fileName) // static --> | |||
<!-- bool QFile::exists() const --> | |||
<function name="QFile::exists"> | |||
<const/> |
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.
This is definitely not const
as it relies on some state. And it is not even pure
as it relies on the filesystem and might return different results at different points in the application.
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.
This is supposed to be member-const, not attribute-const. Overloading that term is very unfortunate.
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.
We have code which use ispure
and isconst
as equal so we have existing could which already mixes this. See CheckAssert::assertWithSideEffects()
.
Also the library loading handles it as "function attribute" const
- see Library::loadFunction()
.
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.
Besides, assertWithSideEffects()
also uses member-const as a sign of no-side-effects in user-declared functions.
A complete mess, as usual.
@@ -5636,6 +5641,7 @@ The obsolete function 'gets' is called. With 'gets' you'll get a buffer overrun | |||
</function> | |||
<!-- int toupper(int c); --> | |||
<function name="toupper,std::toupper"> | |||
<pure/> |
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.
This is not pure
as it depends on the locale which might change during run-time.
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.
Yet we use pure
as our (only) indicator if a function has side effects.
You can also change the floating-point rounding mode, which affects the results of certain math functions...
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.
You can also change the floating-point rounding mode, which affects the results of certain math functions...
True. If you would limit this assumption to only the current scope it seems like you could apply that information but as it depends on a C library function which might be called in another library function that might lead to unexpected effects. I wonder if the compiler will consider this or if this is considered like something as a "data race" or if you are calling chdir()
somewhere in the code meaning that all bets are off.
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.
Maybe this is what the difference between pure
and const
is about? pure
function might be affected by the mutable global state as a locale or floating-point modes and const
ones are not.
That might open up another (whole program analysis) check which would warn if you use pure
functions and any of these global state changing functions in your code base.
@@ -8594,6 +8601,7 @@ initializer list (7) string& replace (const_iterator i1, const_iterator i2, init | |||
<!-- bool std::map::contains( const Key& key ) const; // since C++20 --> | |||
<!-- template< class K > bool std::map::contains( const K& x ) const; // since C++20 --> | |||
<function name="std::map::contains"> | |||
<const/> |
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.
This is not "function attribute" const
.
@@ -5926,6 +5932,7 @@ The function 'mktemp' is considered to be dangerous due to race conditions and s | |||
<!-- http://pubs.opengroup.org/onlinepubs/000095399/functions/sigismember.html --> | |||
<!-- int sigismember(const sigset_t *set, int signum);--> | |||
<function name="sigismember"> | |||
<pure/> |
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.
This is not pure
as the object pointed to might change during run-time.
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.
Edit: moved below
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.
Wrong comment.
@@ -5456,6 +5461,7 @@ The function 'mktemp' is considered to be dangerous due to race conditions and s | |||
<!-- see http://man7.org/linux/man-pages/man3/strnlen.3.html--> | |||
<!-- size_t strnlen(const char *s, size_t maxlen); --> | |||
<function name="strnlen,wcsnlen"> | |||
<pure/> |
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.
This is marked pure
in the standard header but I do not understand how it could be.
Consider this (horrible) example:
int len = strlen(p);
while (len > 0)
{
p[len-1] = '\0';
len = strlen(p);
}
The result of strlen()
on the pointer would obviously be different on each call. So if the compiler would eliminate multiple calls to it we would get unexpected results.
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.
strlen()
is designated as a pure function here: https://gcc.gnu.org/onlinedocs/gcc/Common-Function-Attributes.html
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.
The compiler somehow seems to know that it cannot be called less: https://godbolt.org/z/dKnfPreh7.
So if there is some additional logic involved within the compiler this attribute seems worthless (apart from implying nodiscard
) in cases where we could apply that (like a possibly variableScope
extension).
There is a proposal for [[pure]]]
:
https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2015/p0078r0.pdf. That seems to get into the case where you pass mutable data into a pure function.
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.
I assume the compiler might check if the input is somehow modified and leverages that information to make the decision.
Reading that paper I think it might sense to define our pure
in the way section 6 Redefining pure
does and even create a check for this.
I will also file a clang-tidy ticket about this upstream.
How do we proceed here? @danmar @orbitcowboy
|
No description provided.