C++20 #168

andresailer · 2023-05-22T14:08:43Z

BEGINRELEASENOTES

Pregenerated Headers: remove self-include from some headers (breaks include-what-you-use)
LCIterator, LCRTRelations: remove template syntax causing errors in gcc13/c++20
RunEvent, LCObject, TrackStateImpl: added default copy and move constructor and assignment operator to avoid error about "'definition of implicit copy constructor for 'LCObject' is deprecated because it has a user-declared destructor'"

ENDRELEASENOTES

Fixes for example

In file included from /build/jenkins/workspace/lcg_nightly_pipeline/build/projects/LCIO-02.16.01/src/LCIO/02.16.01/src/cpp/include/pre-generated/EVENT/LCObject.h:10,
                 from /build/jenkins/workspace/lcg_nightly_pipeline/build/projects/LCIO-02.16.01/src/LCIO/02.16.01/src/cpp/include/IMPL/LCCollectionVec.h:8,
                 from /build/jenkins/workspace/lcg_nightly_pipeline/build/projects/LCIO-02.16.01/src/LCIO/02.16.01/src/cpp/src/IMPL/LCCollectionVec.cc:2:
/build/jenkins/workspace/lcg_nightly_pipeline/build/projects/LCIO-02.16.01/src/LCIO/02.16.01/src/cpp/include/LCRTRelations.h:72:33: error: expected unqualified-id before 'const'
   72 |     LCBaseTraits<U, T, I, D, b>(const LCBaseTraits&) = delete ;
      |                                 ^~~~~
/build/jenkins/workspace/lcg_nightly_pipeline/build/projects/LCIO-02.16.01/src/LCIO/02.16.01/src/cpp/include/LCRTRelations.h:72:33: error: expected ')' before 'const'
   72 |     LCBaseTraits<U, T, I, D, b>(const LCBaseTraits&) = delete ;
      |                                ~^~~~~
      |                                 )
/build/jenkins/workspace/lcg_nightly_pipeline/build/projects/LCIO-02.16.01/src/LCIO/02.16.01/src/cpp/include/LCRTRelations.h:82:12: error: template-id not allowed for destructor
   82 |     inline ~LCBaseTraits<U, T, I, D, b>() {
      |            ^
/build/jenkins/workspace/lcg_nightly_pipeline/build/projects/LCIO-02.16.01/src/LCIO/02.16.01/src/cpp/include/LCRTRelations.h:271:23: error: expected unqualified-id before ')' token
  271 |     LCIntExtension<U>() = default ;
      |                       ^
/build/jenkins/workspace/lcg_nightly_pipeline/build/projects/LCIO-02.16.01/src/LCIO/02.16.01/src/cpp/include/LCRTRelations.h:275:5: error: template-id not allowed for destructor
  275 |     ~LCIntExtension<U>() = default ;
      |     ^
/build/jenkins/workspace/lcg_nightly_pipeline/build/projects/LCIO-02.16.01/src/LCIO/02.16.01/src/cpp/include/LCRTRelations.h:304:25: error: expected unqualified-id before ')' token
  304 |     LCFloatExtension<U>() = default ;
      |                         ^
/build/jenkins/workspace/lcg_nightly_pipeline/build/projects/LCIO-02.16.01/src/LCIO/02.16.01/src/cpp/include/LCRTRelations.h:308:5: error: template-id not allowed for destructor
  308 |     ~LCFloatExtension<U>() = default ;
      |     ^
/build/jenkins/workspace/lcg_nightly_pipeline/build/projects/LCIO-02.16.01/src/LCIO/02.16.01/src/cpp/include/LCRTRelations.h:330:24: error: expected unqualified-id before ')' token
  330 |     LCBoolExtension<U>() = default ;
      |                        ^
/build/jenkins/workspace/lcg_nightly_pipeline/build/projects/LCIO-02.16.01/src/LCIO/02.16.01/src/cpp/include/LCRTRelations.h:334:5: error: template-id not allowed for destructor
  334 |     ~LCBoolExtension<U>() = default ;
      |     ^

…perator fix warning -Wdeprecated-copy-with-dtor

Needed to fix 'definition of implicit copy constructor for 'LCObject' is deprecated because it has a user-declared destructor'

tmadlener

Already looks good. Just some nit-picking from my side since we are already at it.

tmadlener · 2023-05-24T12:34:34Z

src/cpp/include/pre-generated/EVENT/LCObject.h

+    LCObject() = default ;
+    LCObject(LCObject const&) = default ;
+    LCObject& operator=(LCObject const&) = default ;
+


What about the move constructors here?

There was no error about missing or implicitly defined move constructors.
Should I add them with default?

I think it's not entirely trivial in this case on whether it will be implicitly defaulted, or implicitly deleted in the cases that we have here: https://en.cppreference.com/w/cpp/language/move_constructor

For LCObject and in general for all the abstract interfaces I think explicitly defaulting them should be fine, IIUC. (At least then they should not prevent anything in the Impl classes).

I added the default move constructors.

tmadlener · 2023-05-24T12:34:38Z

src/cpp/include/pre-generated/EVENT/TrackState.h

@@ -27,8 +27,7 @@ class TrackState : public LCObject {

 public: 
    /// Destructor.
-    virtual ~TrackState() = default;
-
+    //virtual ~TrackState() { /* nop */; } // implicit!


Suggested change

//virtual ~TrackState() { /* nop */; } // implicit!

//virtual ~TrackState() = default; // implicit!

Personal preference for indicating that this is no-op. On the other hand we could also entirely remove the commented one here, c.f. rule of 0.

Apart from the commenting, this change is because I didn't start from the HEAD of master and the line was actually changed, and when I solved the conflict I just took the line from my changes. Yes, completely removing is fine of course

PS: cf. is one word, cf. https://en.wikipedia.org/wiki/Cf. /nitpick

Removed the commented line

tmadlener · 2023-05-24T12:34:59Z

src/cpp/include/SIO/RunEventMap.h

@@ -15,6 +15,8 @@ namespace SIO {
    typedef long long long64 ;

    RunEvent() = default ;
+    RunEvent(RunEvent const&) = default ;
+    RunEvent& operator=(RunEvent const&) = default ;


What about move constructors here?

tmadlener · 2023-05-24T12:35:25Z

src/cpp/include/IMPL/TrackStateImpl.h

@@ -27,6 +27,8 @@ namespace IMPL {
    /** Default constructor, initializes values to 0.
     */
    TrackStateImpl() ;
+    TrackStateImpl(TrackStateImpl const&) = default ;
+    TrackStateImpl& operator=(TrackStateImpl const&) = default ;


Different class, same question: What about move constructors here?

…nt operator implicit as well

andresailer force-pushed the c++20 branch from c068345 to c98d139 Compare May 22, 2023 15:46

andresailer added 6 commits May 23, 2023 08:46

PreGenerated Headers: remove self include (breaks include-what-you-use)

424bc23

Fix template syntax that is no longer valid for gcc13/c++20

7a308a1

SIO::RunEvent: add explicit default copy constructor and assignment o…

e1f6c1c

…perator fix warning -Wdeprecated-copy-with-dtor

TrackStateImple: explicitly add copy ctor, assignment operator

346f709

LCObject: explicitly add copy ctor, assignment operator

e4ae465

Needed to fix 'definition of implicit copy constructor for 'LCObject' is deprecated because it has a user-declared destructor'

Tests: ignore warnings from CLHEP headers

eb72035

andresailer force-pushed the c++20 branch from 5934e39 to 6c4bdc8 Compare May 23, 2023 06:46

tmadlener reviewed May 24, 2023

View reviewed changes

andresailer added 2 commits May 25, 2023 09:56

TrackState: use implicit dtor, so we can have the copy ctor, assignme…

e76c000

…nt operator implicit as well

TrackStateImple, RunEvent, LCObject: add move constructor

3d621e8

andresailer force-pushed the c++20 branch from 6c4bdc8 to 3d621e8 Compare May 25, 2023 08:35

tmadlener approved these changes May 30, 2023

View reviewed changes

tmadlener merged commit da3bd4a into iLCSoft:master May 30, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

C++20 #168

C++20 #168

andresailer commented May 22, 2023 •

edited

Loading

tmadlener left a comment

tmadlener May 24, 2023

andresailer May 24, 2023

tmadlener May 24, 2023

andresailer May 25, 2023

tmadlener May 24, 2023

andresailer May 24, 2023

andresailer May 25, 2023

tmadlener May 24, 2023

andresailer May 25, 2023

tmadlener May 24, 2023

andresailer May 25, 2023

	//virtual ~TrackState() { /* nop */; } // implicit!
	//virtual ~TrackState() = default; // implicit!

C++20 #168

C++20 #168

Conversation

andresailer commented May 22, 2023 • edited Loading

tmadlener left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andresailer commented May 22, 2023 •

edited

Loading