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

Removed old-style-cast from Plumed.h #1090

Merged
merged 6 commits into from
Jul 24, 2024

Conversation

Iximiel
Copy link
Member

@Iximiel Iximiel commented Jun 27, 2024

Description

I removed the old-style casts in Plumed.h.

Now when Plumed.h is in "c++ mode" will use static_cast or reintrpet_cast

I used -Werror=old-style-cast to find the casts and i changed to static when gcc suggested to change to static and reintepret otherwise or when no suggestion were made.

I am not 100% sure to have changed every casts, but at least the ones that affects the gromacs compilation are done plus some more found with searching \(.*?\)\w.

Target release

I would like my code to appear in release 2.10

Type of contribution
  • changes to code or doc authored by PLUMED developers, or additions of code in the core or within the default modules
  • changes to a module not authored by you
  • new module contribution or edit of a module authored by you
Copyright
  • I agree to transfer the copyright of the code I have written to the PLUMED developers or to the author of the code I am modifying.
  • the module I added or modified contains a COPYRIGHT file with the correct license information. Code should be released under an open source license. I also used the command cd src && ./header.sh mymodulename in order to make sure the headers of the module are correct.
Tests
  • I added a new regtest or modified an existing regtest to validate my changes.
  • I verified that all regtests are passed successfully on GitHub Actions.

@GiovanniBussi
Copy link
Member

For clarity, I would:

  • remove __PLUMED_WRAPPER_CAST
  • replace the few places where it is used with reinterpret_cast

Indeed, I see you only use it in regions of the code that are C++ only, so no reason to use this macro

Thanks!

@Iximiel
Copy link
Member Author

Iximiel commented Jun 27, 2024

Indeed, I see you only use it in regions of the code that are C++ only, so no reason to use this macro

I changed it in the std::filesystem part
but some of them are in the f2c, c2f, c2v and v2c functions that are wrapped by __PLUMED_WRAPPER_C_BEGIN/END, are also these only c++?

Then I run astyle, and should be ready

@GiovanniBussi
Copy link
Member

You are right, those are also for C. So we can either keep it as is or maybe you can put explicit reinterpret_cast in the filesystem part, just because it's clearer...

@Iximiel
Copy link
Member Author

Iximiel commented Jun 27, 2024

You are right, those are also for C. So we can either keep it as is or maybe you can put explicit reinterpret_cast in the filesystem part, just because it's clearer...

i changed CAST to REINTERPRET_CAST, so it is clearer, and in the c++ parts I wrote it explicitly

@Iximiel Iximiel marked this pull request as ready for review June 27, 2024 16:55
@Iximiel
Copy link
Member Author

Iximiel commented Jun 28, 2024

The changes should not be accepted, using clang12 for compiling raises a warning about some of the cast to be undefined behaviour. I think I should address this before considering the work done

@Iximiel Iximiel marked this pull request as draft June 28, 2024 06:37
@Iximiel
Copy link
Member Author

Iximiel commented Jun 28, 2024

I think I solved the issue, I do not get anymore the UB warnings. And the solution should be compatible with C++98

@GiovanniBussi
Copy link
Member

@Iximiel I think I can merge this, but you should first make it "not draft" I think

@Iximiel Iximiel marked this pull request as ready for review June 28, 2024 16:16
@GiovanniBussi GiovanniBussi merged commit 3e1bc05 into plumed:master Jul 24, 2024
22 checks passed
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