Skip to content

Add pure/const function attribute (refs #12695) #6385

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.

Sign up for GitHub

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
chrchr-github wants to merge 7 commits into
base: main
Choose a base branch
from
Draft

Add pure/const function attribute (refs #12695) #6385

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
d8c2823
Update posix.cfg
chrchr-github May 6, 2024
841a225
Update std.cfg
chrchr-github May 6, 2024
60e4e71
Update qt.cfg
chrchr-github May 6, 2024
eef3f06
Update std.cfg
chrchr-github May 6, 2024
ff88641
Update std.cfg
chrchr-github May 6, 2024
25f1b4b
Update qt.cfg
chrchr-github May 6, 2024
d9b1f42
Update qt.cfg
chrchr-github May 6, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions cfg/posix.cfg
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
<!-- https://pubs.opengroup.org/onlinepubs/9699919799/functions/wait.html -->
<!-- Note: These are defined as macros in POSIX system headers -->
<function name="WIFEXITED,WEXITSTATUS,WIFSIGNALED,WTERMSIG,WIFSTOPPED,WSTOPSIG,WIFCONTINUED">
<pure/>
<returnValue type="int"/>
<noreturn>false</noreturn>
<leak-ignore/>
Expand Down Expand Up @@ -2901,6 +2902,7 @@ The function 'mktemp' is considered to be dangerous due to race conditions and s
<!-- http://pubs.opengroup.org/onlinepubs/009695399/basedefs/arpa/inet.h.html -->
<!-- uint32_t htonl(uint32_t); -->
<function name="htonl">
<pure/>
<returnValue type="uint32_t"/>
<noreturn>false</noreturn>
<use-retval/>
Expand All @@ -2911,6 +2913,7 @@ The function 'mktemp' is considered to be dangerous due to race conditions and s
</function>
<!-- uint16_t htons(uint16_t); -->
<function name="htons">
<pure/>
<returnValue type="uint16_t"/>
<noreturn>false</noreturn>
<use-retval/>
Expand All @@ -2921,6 +2924,7 @@ The function 'mktemp' is considered to be dangerous due to race conditions and s
</function>
<!-- uint32_t ntohl(uint32_t); -->
<function name="ntohl">
<pure/>
<returnValue type="uint32_t"/>
<noreturn>false</noreturn>
<use-retval/>
Expand All @@ -2931,6 +2935,7 @@ The function 'mktemp' is considered to be dangerous due to race conditions and s
</function>
<!-- uint16_t ntohs(uint16_t); -->
<function name="ntohs">
<pure/>
<returnValue type="uint16_t"/>
<noreturn>false</noreturn>
<use-retval/>
Expand Down Expand Up @@ -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/>
Copy link
Copy Markdown
Collaborator

@firewave firewave May 6, 2024 •
edited
Loading

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.

Copy link
Copy Markdown
Collaborator Author

@chrchr-github chrchr-github May 6, 2024

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

Copy link
Copy Markdown
Collaborator

@firewave firewave May 6, 2024

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.

Copy link
Copy Markdown
Collaborator

@firewave firewave May 6, 2024

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.

<use-retval/>
<returnValue type="size_t"/>
<noreturn>false</noreturn>
Expand Down Expand Up @@ -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/>
Copy link
Copy Markdown
Collaborator

@firewave firewave May 6, 2024

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.

Copy link
Copy Markdown
Collaborator Author

@chrchr-github chrchr-github May 6, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Edit: moved below

Copy link
Copy Markdown
Collaborator

@firewave firewave May 6, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wrong comment.

<returnValue type="int"/>
<noreturn>false</noreturn>
<leak-ignore/>
Expand Down Expand Up @@ -6174,6 +6181,7 @@ The function 'mktemp' is considered to be dangerous due to race conditions and s
<!-- These are defined as macros http://man7.org/linux/man-pages/man7/inode.7.html -->
<!-- It is better to configure them as functions -->
<function name="S_ISREG,S_ISDIR,S_ISCHR,S_ISBLK,S_ISFIFO,S_ISLNK,S_ISSOCK">
<pure/>
<returnValue type="int"/>
<use-retval/>
<noreturn>false</noreturn>
Expand Down
7 changes: 6 additions & 1 deletion cfg/qt.cfg
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,7 @@
<define name="SLOT(X)" value="#X"/>
<!-- T qAbs(const T &t) -->
<function name="qAbs">
<pure/>
<noreturn>false</noreturn>
<use-retval/>
<arg nr="1" direction="in">
Expand Down Expand Up @@ -147,6 +148,7 @@
<!-- bool qFuzzyIsNull(double d) -->
<!-- bool qFuzzyIsNull(float f) -->
<function name="qFuzzyIsNull">
<pure/>
<noreturn>false</noreturn>
<returnValue type="bool"/>
<use-retval/>
Expand Down Expand Up @@ -2337,7 +2339,8 @@
</function>
<!-- bool QDir::exists(QString &name) const -->
<!-- bool QDir::exists() const -->
<function name="QDir">
<function name="QDir::exists">
<const/>
<noreturn>false</noreturn>
<returnValue type="bool"/>
<use-retval/>
Expand All @@ -2357,6 +2360,7 @@
<!-- bool QFile::exists(const QString &fileName) // static -->
<!-- bool QFile::exists() const -->
<function name="QFile::exists">
<const/>
Copy link
Copy Markdown
Collaborator

@firewave firewave May 6, 2024

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.

Copy link
Copy Markdown
Collaborator Author

@chrchr-github chrchr-github May 6, 2024

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.

Copy link
Copy Markdown
Collaborator

@firewave firewave May 6, 2024

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

Copy link
Copy Markdown
Collaborator Author

@chrchr-github chrchr-github May 6, 2024

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.

<noreturn>false</noreturn>
<returnValue type="bool"/>
<use-retval/>
Expand Down Expand Up @@ -2722,6 +2726,7 @@
<!-- bool QDate::isValid() const -->
<!-- bool QDate::isValid(int year, int month, int day) // static -->
<function name="QDate::isValid">
<pure/>
<noreturn>false</noreturn>
<returnValue type="bool"/>
<use-retval/>
Expand Down
8 changes: 8 additions & 0 deletions cfg/std.cfg
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3253,6 +3253,7 @@ The obsolete function 'gets' is called. With 'gets' you'll get a buffer overrun
</function>
<!-- double log(double x); -->
<function name="log,std::log">
<pure/>
<use-retval/>
<returnValue type="double">log(arg1)</returnValue>
<noreturn>false</noreturn>
Expand All @@ -3264,6 +3265,7 @@ The obsolete function 'gets' is called. With 'gets' you'll get a buffer overrun
</function>
<!-- float logf(float x); -->
<function name="logf,std::logf">
<pure/>
<use-retval/>
<returnValue type="float">log(arg1)</returnValue>
<noreturn>false</noreturn>
Expand All @@ -3275,6 +3277,7 @@ The obsolete function 'gets' is called. With 'gets' you'll get a buffer overrun
</function>
<!-- long double logl(long double x);-->
<function name="logl,std::logl">
<pure/>
<use-retval/>
<returnValue type="long double">log(arg1)</returnValue>
<noreturn>false</noreturn>
Expand All @@ -3288,6 +3291,7 @@ The obsolete function 'gets' is called. With 'gets' you'll get a buffer overrun
<!-- float complex clogf(float complex x); -->
<!-- long double complex clogl(long double complex x);-->
<function name="clog,clogf,clogl">
<pure/>
<use-retval/>
<noreturn>false</noreturn>
<leak-ignore/>
Expand All @@ -3299,6 +3303,7 @@ The obsolete function 'gets' is called. With 'gets' you'll get a buffer overrun
<!-- float complex conjf(float complex x);-->
<!-- long double complex conjl(long double complex x);-->
<function name="conj,conjf,conjl">
<pure/>
<use-retval/>
<noreturn>false</noreturn>
<leak-ignore/>
Expand Down Expand Up @@ -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/>
Copy link
Copy Markdown
Collaborator

@firewave firewave May 6, 2024

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.

Copy link
Copy Markdown
Collaborator Author

@chrchr-github chrchr-github May 6, 2024

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

Copy link
Copy Markdown
Collaborator

@firewave firewave May 6, 2024

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.

Copy link
Copy Markdown
Collaborator

@firewave firewave May 6, 2024

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.

<use-retval/>
<returnValue type="int">arg1 &lt; 'a' || arg1 &gt; 'z' ? arg1 : arg1 - 32</returnValue>
<noreturn>false</noreturn>
Expand All @@ -5648,6 +5654,7 @@ The obsolete function 'gets' is called. With 'gets' you'll get a buffer overrun
</function>
<!-- typeid operator -->
<function name="typeid">
<pure/>
<use-retval/>
<noreturn>false</noreturn>
<arg nr="1"/>
Expand Down Expand Up @@ -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/>
Copy link
Copy Markdown
Collaborator

@firewave firewave May 6, 2024

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.

<noreturn>false</noreturn>
<returnValue type="bool"/>
<use-retval/>
Expand Down