plumed2 icon indicating copy to clipboard operation
plumed2 copied to clipboard

Removed old-style-cast from Plumed.h

Open Iximiel opened this issue 1 year ago • 7 comments

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
  • [X] 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
  • [X] 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.

Iximiel avatar Jun 27 '24 07:06 Iximiel

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!

GiovanniBussi avatar Jun 27 '24 07:06 GiovanniBussi

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

Iximiel avatar Jun 27 '24 08:06 Iximiel

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...

GiovanniBussi avatar Jun 27 '24 08:06 GiovanniBussi

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 avatar Jun 27 '24 08:06 Iximiel

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 avatar Jun 28 '24 06:06 Iximiel

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

Iximiel avatar Jun 28 '24 12:06 Iximiel

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

GiovanniBussi avatar Jun 28 '24 13:06 GiovanniBussi