Do not use [[nodiscard]] attribute on compiler not supporting it.

[mgautierfr]

On aarch64, we use gcc version 6.3.0 which doesn't support the [[nodiscard]] attribute
(see https://en.cppreference.com/w/cpp/compiler_support/17)

So don't set the attribute if the attribute is not present.

[veloman-yunkan]

The NODISCARD macro is introduced under the implicit assumption that libkiwix is the only project having run into the portability issue of direct usage of the [[nodiscard]] C++ attribute (I mean what if e.g. xapian defines such a macro too?)

I see two reasonable options out of the current situation:

1. Drop the (only current usage of the) [[nodiscard]] attribute.
2. Think about supporting it in a better way and define the appropriate macro (along with other useful general purpose macros) in a header file of its own (which may better belong to libzim).

[veloman-yunkan]

Shouldn't this be __has_cpp_attribute for better portability (and consistency with the above check)?

[mgautierfr]

Yes

[mgautierfr]

What about renaming NODISCARD to LIBKIWIX_NODISCARD ?

(which may better belong to libzim).

The frontier is a bit blurry but we want to keep the two projects separated.
I don't think it is a good idea to define things in libzim based on compiler features to be used only on libkiwix.

[veloman-yunkan]

What about renaming NODISCARD to LIBKIWIX_NODISCARD ?

That will do, but the next question is "Why does it have to be defined in library.h?"

(which may better belong to libzim).

The frontier is a bit blurry but we want to keep the two projects separated. I don't think it is a good idea to define things in libzim based on compiler features to be used only on libkiwix.

I don't see why the usage of [[nodiscard]] should be limited to libkiwix. If that new C++ feature is a useful one it should be available across all our C++ projects. Hence libzim is the right place for it (until we split out another library for general purpose stuff, something like our own STL/boost/etc).

[mgautierfr]

That will do, but the next question is "Why does it have to be defined in library.h?"

It doesn't have to be defined here, but it is used only in library.h so why should we define elsewhere ?

I don't see why the usage of [[nodiscard]] should be limited to libkiwix. If that new C++ feature is a useful one it should be available across all our C++ projects. Hence libzim is the right place for it (until we split out another library for general purpose stuff, something like our own STL/boost/etc).

The purpose of libzim is not to provide useful things to other projects. It is to provide reading and writing zim files (and even that, we want to split libzim in two libraries openzim/libzim#789)

Drop the (only current usage of the) [[nodiscard]] attribute.

Droping the [[nodiscard]] attribute is also a good solution which avoid a lot of questions.

[veloman-yunkan]

That will do, but the next question is "Why does it have to be defined in library.h?"

It doesn't have to be defined here, but it is used only in library.h so why should we define elsewhere ?

So that it is easier to reuse if/when needed somewhere else. If you care only about Library::create() then consider #undefing the macro after it serves its purpose.

[mgautierfr]

New version:

• Use of LIBKIWIX_NODISCARD
• Undef after use.
• Fix __has_cpp_attribute vs __has_attribute (the former won)

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 38.92%. Comparing base (da89169) to head (01aa190).
Report is 226 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #1003   +/-   ##
=======================================
  Coverage   38.92%   38.92%           
=======================================
  Files          58       58           
  Lines        3990     3990           
  Branches     2201     2201           
=======================================
  Hits         1553     1553           
  Misses       1090     1090           
  Partials     1347     1347           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.