kiwix / libkiwix

Common code base for all Kiwix ports
https://download.kiwix.org/release/libkiwix/
GNU General Public License v3.0
118 stars 56 forks source link

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

Closed mgautierfr closed 11 months ago

mgautierfr commented 11 months ago

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.

mgautierfr commented 11 months ago

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

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

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 https://github.com/openzim/libzim/issues/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 commented 11 months ago

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

New version:

codecov[bot] commented 11 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

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

Additional details and impacted files ```diff @@ 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 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.