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

zajo commented 4 months ago

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 commented 4 months ago

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 commented 4 months ago

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 commented 4 months ago

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 commented 4 months ago

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 commented 4 months ago

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 commented 4 months ago

Yes, the issue is in the nodiscard macro.

indiosmo commented 1 month ago

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 commented 1 month ago

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.