FairRootGroup / FairRoot

C++ simulation, reconstruction and analysis framework for particle physics experiments
http://fairroot.gsi.de
Other
57 stars 96 forks source link

chore: Drop Fair{ParSet,Container} forward decls #1457

Closed ChristianTackeGSI closed 8 months ago

ChristianTackeGSI commented 1 year ago

and add some override, while in those files.


Checklist:

fuhlig1 commented 1 year ago

@ChristianTackeGSI,

why do you delete the forward declarations? The classes are used in the code, so in my opinion they belong there even if they come by chance via the include statement.

ChristianTackeGSI commented 1 year ago

Most places use those classes in overriden methods. So that original declaration of that method must have the right declarations already. So those declarations do not come "by chance".

And in the examples, we should not use forward declarations of main FairRoot classes. We should use the correct #includes in those cases: Users should learn to use correct includes and not forward declarations for FairRoot classes. For example we could switch classes to usings with the exact same API. But usings and classes are not the same.

YanzhaoW commented 1 year ago

a subtle comment: don't default a destructor if it can be removed.

There is always a preference for "Rule of 0", which means no definition of any special member functions (destructor, move constructor, copy constructor, move assignment, copy assignment). If any of them need to be defined, they should be defined all together. (Rule of 5)

ChristianTackeGSI commented 1 year ago

Thanks for your comments!

I think, the relevant one would rather be the "Rule of 5": At least the default constructor needs to be defined in these cases. So we probably should declare them all (even if only = defaulted, to be explicit).

So "being explicit" is the thing here. And I wanted to start with the desctructor that was already there.

Of course, this stuff could be moved into its own commit, maybe. Let me take another look.

ChristianTackeGSI commented 1 year ago

Moved the override and =default into its own commit.

coderabbitai[bot] commented 8 months ago

Walkthrough

The updates across various files primarily involve copyright year adjustments and modernizing the class definitions within the C++ codebase. Key modifications include defaulting destructors and marking them as override, transitioning from ClassDef to ClassDefOverride for class definitions, and explicitly marking certain methods as override. These changes streamline the code and adhere to modern C++ best practices, ensuring consistency and clarity in the management of class inheritance and object destruction.

Changes

File Pattern Change Summary
.../FairPassiveContFact.h,
.../FairTutorialDet[1-4]ContFact.h,
.../FairRutherfordContFact.h,
.../NewDetectorContFact.h (across various paths),
.../MyPassiveContFact.h (across various paths)
- Copyright year updated
- Destructor defaulted and marked as override
- createContainer method marked as override
- Transition from ClassDef to ClassDefOverride

Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?

Share - [X](https://twitter.com/intent/tweet?text=I%20just%20used%20%40coderabbitai%20for%20my%20code%20review%2C%20and%20it%27s%20fantastic%21%20It%27s%20free%20for%20OSS%20and%20offers%20a%20free%20trial%20for%20the%20proprietary%20code.%20Check%20it%20out%3A&url=https%3A//coderabbit.ai) - [Mastodon](https://mastodon.social/share?text=I%20just%20used%20%40coderabbitai%20for%20my%20code%20review%2C%20and%20it%27s%20fantastic%21%20It%27s%20free%20for%20OSS%20and%20offers%20a%20free%20trial%20for%20the%20proprietary%20code.%20Check%20it%20out%3A%20https%3A%2F%2Fcoderabbit.ai) - [Reddit](https://www.reddit.com/submit?title=Great%20tool%20for%20code%20review%20-%20CodeRabbit&text=I%20just%20used%20CodeRabbit%20for%20my%20code%20review%2C%20and%20it%27s%20fantastic%21%20It%27s%20free%20for%20OSS%20and%20offers%20a%20free%20trial%20for%20proprietary%20code.%20Check%20it%20out%3A%20https%3A//coderabbit.ai) - [LinkedIn](https://www.linkedin.com/sharing/share-offsite/?url=https%3A%2F%2Fcoderabbit.ai&mini=true&title=Great%20tool%20for%20code%20review%20-%20CodeRabbit&summary=I%20just%20used%20CodeRabbit%20for%20my%20code%20review%2C%20and%20it%27s%20fantastic%21%20It%27s%20free%20for%20OSS%20and%20offers%20a%20free%20trial%20for%20proprietary%20code)

Tips ### Chat There are 3 ways to chat with CodeRabbit: - Review comments: Directly reply to a review comment made by CodeRabbit. Example: - `I pushed a fix in commit .` - `Generate unit-tests for this file.` - Files and specific lines of code (under the "Files changed" tab): Tag `@coderabbitai` in a new review comment at the desired location with your query. Examples: - `@coderabbitai generate unit tests for this file.` - `@coderabbitai modularize this function.` - PR comments: Tag `@coderabbitai` in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples: - `@coderabbitai generate interesting stats about this repository from git and render them as a table.` - `@coderabbitai show all the console.log statements in this repository.` - `@coderabbitai read src/utils.ts and generate unit tests.` - `@coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.` Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. ### CodeRabbit Commands (invoked as PR comments) - `@coderabbitai pause` to pause the reviews on a PR. - `@coderabbitai resume` to resume the paused reviews. - `@coderabbitai review` to trigger a review. This is useful when automatic reviews are disabled for the repository. - `@coderabbitai resolve` resolve all the CodeRabbit review comments. - `@coderabbitai help` to get help. Additionally, you can add `@coderabbitai ignore` anywhere in the PR description to prevent this PR from being reviewed. ### CodeRabbit Configration File (`.coderabbit.yaml`) - You can programmatically configure CodeRabbit by adding a `.coderabbit.yaml` file to the root of your repository. - The JSON schema for the configuration file is available [here](https://coderabbit.ai/integrations/coderabbit-overrides.v2.json). - If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: `# yaml-language-server: $schema=https://coderabbit.ai/integrations/coderabbit-overrides.v2.json` ### CodeRabbit Discord Community Join our [Discord Community](https://discord.com/invite/GsXnASn26c) to get help, request features, and share feedback.