Skip to content
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.

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

kernel: upgrade to C++ 17 #1289

Merged
merged 1 commit into from
Aug 20, 2024

Conversation

wkozaczuk
Copy link
Collaborator

@wkozaczuk wkozaczuk commented Dec 19, 2023

This patch makes necessary changes to the relevant source files to make it possible to build kernel at C++ 17 level. This will allow us to take advantage of new features available in C++ 17 standard.

The following changes are part of this PR:

  • remove obsolete register keyword (mostly under bsd/ part of the tree)

  • add const specifier to the comparison operator definitions where necessary

  • replace deprecated std::iterator usage with explicit definitions of traits (for example iterator_category, value_type, and 3 others) (for some background info read this - https://www.fluentcpp.com/2018/05/08/std-iterator-deprecated/)

  • add constexpr to the friend declaration of std::distance<mbuf_iterator>

  • make tests, httpserver-api, and some other modules compilable with non-C++ 11 level
    Please note, that all the unit tests and modules implemented in C++ are not subject to this change. On that note, the unit tests are passing.

An example of how to build with C++ 17:

./scripts/build image=httpserver-api fs=rofs.fg -j$(nproc) conf_cxx_level=gnu++17

Finally, this PR does not change the default C++ level which remains at 11, until we permanently decide to change in the future.

@wkozaczuk wkozaczuk requested a review from nyh December 19, 2023 21:45
@nyh
Copy link
Contributor

nyh commented Dec 19, 2023

I'm curious, what is the motivation of this change. Which C++17 features are you planning to use?
But I admit it's kind of silly to stick to the 12 year old C++11 standard, so I won't oppose this change.

@wkozaczuk
Copy link
Collaborator Author

@nyh I have some ideas on how to tackle #853 and I needed a way to re-purpose existing waitlist objects and instead of deleting them, keep them (your idea to comment out erase) and "re-key" unused entries by doing extract and insert (see https://www.fluentcpp.com/2020/05/01/how-to-change-a-key-in-a-map-or-set-in-cpp/). This is to minimize allocations.

@wkozaczuk
Copy link
Collaborator Author

I have relied on the compiler to tell me what is wrong and then verify things still work by using unit tests. But based on your experience are there potentially things that may get broken by this change and we would not even know about it? (see 'Changes to evaluation order' or 'Guaranteed copy elision' in https://developers.redhat.com/articles/2021/08/06/porting-your-code-c17-gcc-11)

@wkozaczuk
Copy link
Collaborator Author

@nyh I have updated this PR. The changes include fixes to core/callstack.cc and adding new build config option conf_cxx_level. So the kernel and all relevant modules get built with non-c++_11 only if explicitly specified.

This patch makes necessary changes to the relevant source files to make it
possible to build kernel at C++ 17 level. This will allows us to take
advantage of new features available in C++ 17 standard.

Following changes have been:

- remove obsolete 'register' keyword (mostly under bsd/ part of the
  tree)

- add 'const' specifier to the comparison operator definitions where
  necessary

- replace deprecated 'std::iterator' usage with explicit definition
  of traits (for example iterator_category, value_type and 3 others)
  (for some background info read this -
https://www.fluentcpp.com/2018/05/08/std-iterator-deprecated/)

- add constexpr to the 'friend' declaration of std::distance<mbuf_iterator>

- build using C++17 using following command:

./scripts/build image=tests conf_cxx_level=gnu++17

Signed-off-by: Waldemar Kozaczuk <[email protected]>
@wkozaczuk wkozaczuk merged commit 03d3fb2 into cloudius-systems:master Aug 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants