Skip to content

COMP: Remove circular include dependency#5047

Merged
hjmjohnson merged 1 commit intoInsightSoftwareConsortium:masterfrom
hjmjohnson:remove-circular-include
Dec 12, 2024
Merged

COMP: Remove circular include dependency#5047
hjmjohnson merged 1 commit intoInsightSoftwareConsortium:masterfrom
hjmjohnson:remove-circular-include

Conversation

@hjmjohnson
Copy link
Member

Reduce the number of files that need to be read by the pre-processor and remove the circular include dependancies.

The itkExceptionObject.h file was allowed to be included only by itkMacro.h which always included the file.

Moving the contents of itkExceptionObject.h to
itkMacro.h simplifies the dependency complexity.

itk::ExceptionObject is different from other classes because it depends on macros at the top of itkMacro.h for it's definition, but also is needed for many macros at the bottom of itkMacro.h.

ITK/Modules/Core/Common/include/itkExceptionObject.h:22:12: warning: circular header file dependency detected while including 'itkMacro.h', please check the include path [misc-header-include-cycle]
22 | # include "itkMacro.h"
| ^

PR Checklist

@hjmjohnson hjmjohnson added this to the ITK 6.0 Beta 1 milestone Dec 11, 2024
@hjmjohnson hjmjohnson self-assigned this Dec 11, 2024
@github-actions github-actions bot added type:Compiler Compiler support or related warnings area:Core Issues affecting the Core module labels Dec 11, 2024
@hjmjohnson hjmjohnson force-pushed the remove-circular-include branch from 1c3422c to 35378a0 Compare December 11, 2024 22:24
@blowekamp
Copy link
Member

I think smashing the file into Macro.h will make it less manageable. The circular dependency had been addresses with preprocessor commands. I am not clear what issue this is trying to address. What effect will it have on Doxygen documentation?

@hjmjohnson hjmjohnson force-pushed the remove-circular-include branch 3 times, most recently from 085ae64 to faa1cd4 Compare December 12, 2024 02:08
@hjmjohnson
Copy link
Member Author

@blowekamp I respectfully disagree. I have fought with those header guards and managed the convoluted circular many times. They break static analysis tools that just give up when the circular dependency is encountered. IDE's can fail to identify the circular inclusions, or how to handle them.

It's probably not much of a speed increase, but it requires 3 file reads by the pre-processor rather than 1. (for every object file compilation that uses itkMacro.h).

To me this is the better choice.

@hjmjohnson
Copy link
Member Author

@blowekamp I will try a different approach, but I think it will likely not work out.

@blowekamp
Copy link
Member

blowekamp commented Dec 12, 2024

I have fought with those header guards and managed the convoluted circular many times. They break static analysis tools that just give up when the circular dependency is encountered. IDE's can fail to identify the circular inclusions, or how to handle them.

OK, those seem like reasonable reasons. Performance... not so much :)

But I think there are alternatives. Although I am not sure how the static analysis tools behave.

Perhaps the Macro.h include can be removed from itkExceptionObject, since it is already guarded to ensure itkMacro.h is included? Or add a ifndef itkMacro_h around it.

The macro them selves in itkMacro.h don't require a declaration of ExceptionObject, but I the template function itkDynamicCastInDebugMode does. Perhaps template functions should go some place else? This one could be hacked to the end of itkExceptionObject.h ( so creating another itkTemplateFunctions.h may impact performance 🤣 )?

Perhaps a forward declaration of "itk::ExceptionObject" and a function "ExeceptionObject MakeExceptionObject(Args&&... args)" could be done?

I hacked #1 and #2 in the following patch. I'd would have made a PR to share but don't want to burden the CI.

blowekamp/ITK@354d67e

@blowekamp
Copy link
Member

@blowekamp I will try a different approach, but I think it will likely not work out.

I just did. Please see above and give it a try with your tools.

Copy link
Member

@dzenanz dzenanz left a comment

Choose a reason for hiding this comment

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

Looks reasonable to me.

@dzenanz
Copy link
Member

dzenanz commented Dec 12, 2024

blowekamp@354d67e

This could still have circular dependency, if someone includes itkExceptionObject.h instead of itkMacro.

Reduce the number of files that need to be read by the pre-processor and
remove the circular include dependancies.

The itkExceptionObject.h file is allowed to be included *only* by
itkMacro.h which always included the file.

Moving the inclusion of itkExceptionObject.h to the end of
itkMacro.h simplifies the dependency complexity.

itk::ExceptionObject is different from other classes because it depends
on macros at the top of itkMacro.h for it's definition, but also is
needed for many macros defnined in itkMacro.h.

Removes 10000's of warnings like:
ITK/Modules/Core/Common/include/itkExceptionObject.h:22:12:
warning: circular header file dependency detected while including 'itkMacro.h', please check the include path [misc-header-include-cycle]
   22 | #  include "itkMacro.h"
      |            ^

Co-Authored by: Hans Johnson <hans-johnson@uiowa.edu>
@hjmjohnson hjmjohnson force-pushed the remove-circular-include branch from faa1cd4 to e8c9a0e Compare December 12, 2024 15:03
@hjmjohnson
Copy link
Member Author

@blowekamp Your solution was what I thought about last night! I've take your patch and modified to remove the circular dependency.

I think the old code was overly complicated because:

  1. the include was in the middle of the itkMacro.h file
  2. the header guards tried to use itkExceptionMacro_h for 2 purposes (header guard and include control.

The current patch builds on macos-14 with python turned on. It builds without errors, and it removes circular dependencies.

@blowekamp blowekamp self-requested a review December 12, 2024 15:26
Copy link
Member

@blowekamp blowekamp left a comment

Choose a reason for hiding this comment

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

Thanks for taking a second stab the improvement.

@hjmjohnson hjmjohnson merged commit a6e7085 into InsightSoftwareConsortium:master Dec 12, 2024
@hjmjohnson hjmjohnson deleted the remove-circular-include branch December 12, 2024 18:23
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 type:Compiler Compiler support or related warnings

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants