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

-Wextra-semi #4699

Conversation

andrei-sandor
Copy link
Contributor

@andrei-sandor andrei-sandor commented May 30, 2024

This is part 2 of if the following changes regarding the -Wextra-semi (semi = semicolon) warning of clang can be done throughout the repository. #4679

COMP: Remove ; from itkBooleanMacro

COMP: Remove ; from other itk macros

COMP: Updated itkLegacyMacro Definition. We decided to change the definition of itkLegacyMacro to not have two ; if the if defined(ITK_LEGACY_REMOVE) branch is hit.

COMP: Remove ITK_ASSOCIATE ; or struct;
Should we remove the semicolon from the struct or from ITK_ASSOCIATE?

COMP: Remove ; to function cases

COMP: Remove cases with ; to override cases

COMP: Remove ; IPL cases

@github-actions github-actions bot added type:Compiler Compiler support or related warnings type:Testing Ensure that the purpose of a class is met/the results on a wide set of test cases are correct area:Core Issues affecting the Core module area:Filtering Issues affecting the Filtering module area:IO Issues affecting the IO module area:Registration Issues affecting the Registration module area:Segmentation Issues affecting the Segmentation module area:Numerics Issues affecting the Numerics module labels May 30, 2024
@dzenanz dzenanz requested a review from hjmjohnson May 30, 2024 19:05
@github-actions github-actions bot added the area:Video Issues affecting the Video module label May 30, 2024
@andrei-sandor andrei-sandor changed the title COMP: Remove ; from itkBooleanMacro -Wextra-semi May 30, 2024
@github-actions github-actions bot removed the type:Compiler Compiler support or related warnings label May 30, 2024
@seanm seanm marked this pull request as draft May 30, 2024 21:30
Comment on lines 96 to 101
IPLSetMacroDeclaration(ImageFileName, std::string) IPLGetMacroDeclaration(ImageFileName, std::string)
IPLSetMacroDeclaration(SliceLocation, float) IPLGetMacroDeclaration(SliceLocation, float)
IPLSetMacroDeclaration(SliceOffset, int) IPLGetMacroDeclaration(SliceOffset, int)
IPLSetMacroDeclaration(EchoNumber, int) IPLGetMacroDeclaration(EchoNumber, int)
IPLSetMacroDeclaration(ImageNumber, int) IPLGetMacroDeclaration(ImageNumber, int)
IPLSetMacroDeclaration(Data, void *) IPLGetMacroDeclaration(Data, const void *)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

clang-format screwed this up, you need to add this macro to that list in the .clang-format file to get it to format correctly.

@andrei-sandor andrei-sandor force-pushed the WextraSemiRest branch 2 times, most recently from d256668 to 464715f Compare May 31, 2024 13:15
Comment on lines +96 to +104
IPLSetMacroDeclaration(ImageFileName, std::string)
IPLGetMacroDeclaration(ImageFileName, std::string) IPLSetMacroDeclaration(SliceLocation, float)
IPLGetMacroDeclaration(SliceLocation, float) IPLSetMacroDeclaration(SliceOffset, int)
IPLGetMacroDeclaration(SliceOffset, int) IPLSetMacroDeclaration(EchoNumber, int)
IPLGetMacroDeclaration(EchoNumber, int) IPLSetMacroDeclaration(ImageNumber, int)
IPLGetMacroDeclaration(ImageNumber, int) IPLSetMacroDeclaration(Data, void *)
IPLGetMacroDeclaration(Data, const void *)

private:
std::string m_ImageFileName{};
private : std::string m_ImageFileName{};
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Formatting wrong here too. Hopefully running clang-format will fix it, since you added these macros to StatementMacros.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would really prefer to NOT remove the semi-colons. It makes clang-formatting very challenging and tedious. Additionally, many modern IDE's complain about the lack of ; at the end of what is interpreted as a statement.

I would prefer that the macros are always terminated by a statement, or a NOOP statement as is proposed in #4706

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It makes clang-formatting very challenging and tedious.

How so? Are you perhaps not aware of clang-format's statementmacros setting?

Additionally, many modern IDE's complain about the lack of ; at the end of what is interpreted as a statement.

I've never seen that, could you give an example?

@@ -893,7 +893,7 @@ class ITKCommon_EXPORT ProcessObject : public Object
* updated in derived filters.
*/
itkGetConstMacro(ThreaderUpdateProgress, bool);
itkBooleanMacro(ThreaderUpdateProgress);
itkBooleanMacro(ThreaderUpdateProgress)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Look at the ITK_MACRO_NOOP mechanism.

We want the ; colon here to keep the code consistent. It may be that the we need to update the itkMacro.h to get rid of these.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NOTE: ALSO ENSURE THAT "ITK_LEGACY_REMOVE" and ITK_FUTURE_LEGACY_REMOVE is turned on when running this flag for testing.

Copy link
Contributor

@seanm seanm May 31, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We want the ; colon here to keep the code consistent.

You want the semi colon there? Even though you wouldn't if the macro's equivalent was written out by hand?

Example, itkBooleanMacro(ThreaderUpdateProgress) written manually would be:

  virtual void ThreaderUpdateProgressOn() { this->SetThreaderUpdateProgress(true); }
  virtual void ThreaderUpdateProgressOff() { this->SetThreaderUpdateProgress(false); }

which you wouldn't generally write as:

  virtual void ThreaderUpdateProgressOn() { this->SetThreaderUpdateProgress(true); }
  virtual void ThreaderUpdateProgressOff() { this->SetThreaderUpdateProgress(false); };

As this macro isn't a function-like (or statement-like) macro, it seems odd to end it with a semi.

Comment on lines +96 to +104
IPLSetMacroDeclaration(ImageFileName, std::string)
IPLGetMacroDeclaration(ImageFileName, std::string) IPLSetMacroDeclaration(SliceLocation, float)
IPLGetMacroDeclaration(SliceLocation, float) IPLSetMacroDeclaration(SliceOffset, int)
IPLGetMacroDeclaration(SliceOffset, int) IPLSetMacroDeclaration(EchoNumber, int)
IPLGetMacroDeclaration(EchoNumber, int) IPLSetMacroDeclaration(ImageNumber, int)
IPLGetMacroDeclaration(ImageNumber, int) IPLSetMacroDeclaration(Data, void *)
IPLGetMacroDeclaration(Data, const void *)

private:
std::string m_ImageFileName{};
private : std::string m_ImageFileName{};
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would really prefer to NOT remove the semi-colons. It makes clang-formatting very challenging and tedious. Additionally, many modern IDE's complain about the lack of ; at the end of what is interpreted as a statement.

I would prefer that the macros are always terminated by a statement, or a NOOP statement as is proposed in #4706

@hjmjohnson
Copy link
Member

Closing. I think this work is superceeded with updates made in the master branch that elminiates the need to remove the ";" from macros.

@hjmjohnson hjmjohnson closed this Jun 10, 2024
@seanm
Copy link
Contributor

seanm commented Jun 13, 2024

Some of the commits here are still good, but we'll make a new PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:Core Issues affecting the Core module area:Filtering Issues affecting the Filtering module area:IO Issues affecting the IO module area:Numerics Issues affecting the Numerics module area:Registration Issues affecting the Registration module area:Segmentation Issues affecting the Segmentation module area:Video Issues affecting the Video module type:Testing Ensure that the purpose of a class is met/the results on a wide set of test cases are correct
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants