Add the [[nodiscard]] attribute to the result class

[indiosmo]

[zajo]

The difficulty with these failures is that with LEAF being a standalone library, it doesn't use Boost Config. We need the correct compiler-specific ifdefs in leaf/config.hpp.

[indiosmo]

Not sure I understand, the macro in question is already defined in leaf/config.hpp, line 212.

  #if defined(__has_attribute) && defined(__SUNPRO_CC) && (__SUNPRO_CC > 0x5130)
  #   if __has_attribute(nodiscard)
  #       define BOOST_LEAF_ATTRIBUTE_NODISCARD [[nodiscard]]
  #   endif
  #elif defined(__has_cpp_attribute)
      //clang-6 accepts [[nodiscard]] with -std=c++14, but warns about it -pedantic
  #   if __has_cpp_attribute(nodiscard) && !(defined(__clang__) && (__cplusplus < 201703L)) && !(defined(__GNUC__) && (__cplusplus < 201100))
  #       define BOOST_LEAF_ATTRIBUTE_NODISCARD [[nodiscard]]
  #   endif
  #endif
  #ifndef BOOST_LEAF_ATTRIBUTE_NODISCARD
  #   define BOOST_LEAF_ATTRIBUTE_NODISCARD
  #endif

• edit1 Perhaps you mean this is not sufficient, and boost config handles the missing cases, is that it?

• edit2 I'm looking into Boost Config and the only mention of [[nodiscard]] is in detail/suffix.hpp, and it's exactly the same macro definition that is in leaf/config.hpp so I'm not sure how Boost Config would help here.

[zajo]

I mean the ifdefs are not correct, we need to inspect the error logs to see what's the problem. The goal is to make leaf/config.hpp behave the same as boost/config.hpp (which I presume is correct) in terms of detecting the relevant [nodiscard] support in various compiler versions. I might be able look into it during the weekend.

[indiosmo]

I looked into boost/config.hpp and it seems to be exactly the same as leaf/config.hpp as far as [[nodiscard]] is concerned.

Also, I tried looking at the CI logs but all the ones that failed are blank after line 49.

[zajo]

The logs don't end at line 49, it just takes a while for github to fetch the whole thing. The logs are tens of thousands of lines long, that's why. :)

[indiosmo]

I've reverted the changes to see if all tests pass, as at a cursory look it seems that some of the errors are unrelated to the change. Let's see.

edit: Nevermind, here's a failure for nodiscard specifically:

2024-05-10T16:59:48.1125656Z In file included from ./boost/leaf/result.hpp:9:0, 2024-05-10T16:59:48.1128442Z from libs/leaf/test/_hpp_result_test.cpp:6: 2024-05-10T16:59:48.1129879Z ./boost/leaf/config.hpp:219:47: error: expected identifier before '[' token 2024-05-10T16:59:48.1131442Z # define BOOST_LEAF_ATTRIBUTE_NODISCARD [[nodiscard]]

[zajo]

Yes, the issue is in the nodiscard macro.

[indiosmo]

Hi @zajo, I have some time so I'm revisiting this.

What I'm having trouble with is that the current guard only enables the attribute if compiling with c++17 or above and it still fails for example using gcc 10 regardless of the standard.

2024-05-09T13:44:00.3622608Z gcc.compile.c++ bin.v2/libs/leaf/test/_hpp_result_test.test/gcc-10/debug/x86_64/cxxstd-11-iso/threading-multi/_hpp_result_test.o
2024-05-09T13:44:00.3624449Z In file included from ./boost/leaf/result.hpp:9,
2024-05-09T13:44:00.3625318Z                  from libs/leaf/test/_hpp_result_test.cpp:6:
2024-05-09T13:44:00.3631902Z ./boost/leaf/config.hpp:219:47: error: expected identifier before ‘[’ token
2024-05-09T13:44:00.3633198Z   219 | #       define BOOST_LEAF_ATTRIBUTE_NODISCARD [[nodiscard]]

I don't understand how line 219 is being called when building with -std=c++11 if the guard above it is: # if __has_cpp_attribute(nodiscard) && __cplusplus >= 201703L

Ignoring that for now and looking further:

2024-05-09T13:44:00.3624449Z In file included from ./boost/leaf/result.hpp:9,
2024-05-09T13:44:00.3625318Z                  from libs/leaf/test/_hpp_result_test.cpp:6:
2024-05-09T13:44:00.3631902Z ./boost/leaf/config.hpp:219:47: error: expected identifier before ‘[’ token
2024-05-09T13:44:00.3633198Z   219 | #       define BOOST_LEAF_ATTRIBUTE_NODISCARD [[nodiscard]]
2024-05-09T13:44:00.3633984Z       |                                               ^
2024-05-09T13:44:00.3635764Z ./boost/leaf/result.hpp:177:33: note: in expansion of macro ‘BOOST_LEAF_ATTRIBUTE_NODISCARD’
2024-05-09T13:44:00.3637134Z   177 | class BOOST_LEAF_SYMBOL_VISIBLE BOOST_LEAF_ATTRIBUTE_NODISCARD result
2024-05-09T13:44:00.3637923Z       |                                 ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

Looks like we end up mixing both __attribute__ and [[attribute]] syntax, which doesn't seem to be supported by gcc. class __attribute__((__visibility__("default"))) [[nodiscard]] result

[indiosmo]

Here's some exploration https://godbolt.org/z/c6fWx9KEz

It builds with gcc 7.5 and std=c++11, it fails with c++17 with both attributes, but works with only one of them.

With nodiscard first:

<source>:16:38: error: expected identifier before '__attribute__'
 #   define BOOST_LEAF_SYMBOL_VISIBLE __attribute__((__visibility__("default")))
                                      ^
<source>:21:39: note: in expansion of macro 'BOOST_LEAF_SYMBOL_VISIBLE'
 class  BOOST_LEAF_ATTRIBUTE_NODISCARD BOOST_LEAF_SYMBOL_VISIBLE result

With visible first:

<source>:8:47: error: expected identifier before '[' token
 #       define BOOST_LEAF_ATTRIBUTE_NODISCARD [[nodiscard]]

So the issue is in fact mixing both. Mixing only works starting on GCC 13.1.

Seems to work on all compilers if we use [[gnu:] syntax instead of __attribute__: # define BOOST_LEAF_SYMBOL_VISIBLE [[gnu::visibility("default")]]

I'm commiting this so we can test.

[zajo]

Thanks. And yes, feel free to change the configuration macros if needed.

[indiosmo]

Looks like there are issues with the CI environment:

2024-07-25T19:02:11.9945506Z ##[command]/usr/bin/docker exec  1e5273ba1dd06360032578220225456afc2a23ff95f326ef97318aa6eefbfbd7 sh -c "cat /etc/*release | grep ^ID"
2024-07-25T19:02:12.1407151Z /__e/node20/bin/node: /lib/x86_64-linux-gnu/libc.so.6: version `GLIBC_2.28' not found (required by /__e/node20/bin/node)

[zajo]

Yeah, occasionally old environments get deprecated. I'll look into it.

[zajo]

This is fixed in latest develop, you can integrate that in your branch to re-run gha.

[indiosmo]

Still having issues with the CI environment: test/try_catch_system_error_test.cpp:149:1: fatal error: error writing to /tmp/cc3wazIO.s: No space left on device

[zajo]

Yes, I will fix this weekend, and then I'll merge your PR assuming tests pass. Thank you once again.

[indiosmo]

Thanks @zajo. Is this going to be in for boost 1.86? Do we need to contact anyone about it and to update the release notes?

[zajo]

Wait, why was the visibility definition changed?

[indiosmo]

The visibility value itself was unchanged. What changed was the syntax from __attribute__ to [[gnu:]] since we're using brackets for nodiscard. GCC wouldn't build with mixed attributes syntax up to version 13.

[zajo]

Thanks. I have requested from the release managers to include this in the current release.

[zajo]

The master branch was opened for bug fixes after the beta release, so this will make it in the upcoming Boost release. Thanks once again.