Skip to content

COMP: Avoid Superclass type alias GCC compile errors#2720

Merged
jhlegarreta merged 1 commit intoInsightSoftwareConsortium:masterfrom
thewtex:label-geometry-superclass
Sep 10, 2021
Merged

COMP: Avoid Superclass type alias GCC compile errors#2720
jhlegarreta merged 1 commit intoInsightSoftwareConsortium:masterfrom
thewtex:label-geometry-superclass

Conversation

@thewtex
Copy link
Member

@thewtex thewtex commented Sep 8, 2021

GCC is not resolving object->Superclass in some cases even though the
definition is available and other compilers are resolving the
definition.

xref #2676

@github-actions github-actions bot added area:Core Issues affecting the Core module 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 labels Sep 8, 2021
Copy link
Member

@jhlegarreta jhlegarreta left a comment

Choose a reason for hiding this comment

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

Although I do not have a better proposal (sorry), I am not sure avoiding to print the superclass name is what we would like as a mid-/long-term fix.

I find particularly worrying that this is only happening for a few classes in the review module; the macro is used extensively throughout the code and the faulty statements are working well. So I think there is something more behind it that we are not being able to identify.

As for the proposed fix, if this is what we aim at for now, am I wrong when saying that the only statements that should need to be within the if/else are those related to the superclass? i.e. That the rest of the code is exactly the same, and we could avoid duplicating it?

@Leengit
Copy link
Contributor

Leengit commented Sep 8, 2021

In #2676, the object is a smart pointer rather than a C++ pointer, and perhaps that is the reason that object->Superclass::GetNameOfClass() is confusing to the compiler. Would a simple switch to (*object).Superclass::GetNameOfClass() help? Unfortunately I can't test out this or other solutions on my own system because the original code compiles just fine on my system.

@thewtex
Copy link
Member Author

thewtex commented Sep 8, 2021

Would a simple switch to (*object).Superclass::GetNameOfClass() help?

This results in the same error with GCC 9.3.0.

Copy link
Contributor

@Leengit Leengit left a comment

Choose a reason for hiding this comment

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

Looks good.

@jhlegarreta
Copy link
Member

Windows complaints might have to do with the comments in #2720 (comment):
https://open.cdash.org/viewBuildError.php?buildid=7444074

@Leengit
Copy link
Contributor

Leengit commented Sep 8, 2021

Grasping at straws, but ... in, e.g., Modules/Nonunit/Review/include/itkLabelGeometryImageFilter.h, should itkTypeMacro be invoked before itkNewMacro? That might be helpful if, deep inside, itkNewMacro can print debug output that makes use of the GetNameOfClass method defined by itkTypeMacro. (Yeah, the compiler should be figuring this out, even if out of order, ....)

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.

Would a simple switch to (*object).Superclass::GetNameOfClass() help?

I am trying this on my Linux box.

@dzenanz
Copy link
Member

dzenanz commented Sep 8, 2021

Lee's suggestion does not work:

In file included from /home/dzenan/Dashboards/Tests/ITK/Modules/Nonunit/Review/test/itkLabelGeometryImageFilterTest.cxx:26:
/home/dzenan/Dashboards/Tests/ITK/Modules/Nonunit/Review/test/itkLabelGeometryImageFilterTest.cxx: In function ‘int LabelGeometryImageFilterTest(std::string, std::string, std::string, std::string, std::string)’:
/home/dzenan/Dashboards/Tests/ITK/Modules/Core/TestKernel/include/itkTestingMacros.h:68:53: error: ‘Superclass’ has not been declared
   68 |   std::cout << "Name of Superclass = " << (*object).Superclass::GetNameOfClass() << std::endl;                         \
      |                                                     ^~~~~~~~~~
/home/dzenan/Dashboards/Tests/ITK/Modules/Core/TestKernel/include/itkTestingMacros.h:68:53: note: in definition of macro ‘ITK_EXERCISE_BASIC_OBJECT_METHODS’
   68 |   std::cout << "Name of Superclass = " << (*object).Superclass::GetNameOfClass() << std::endl;                         \
      |                                                     ^~~~~~~~~~
/home/dzenan/Dashboards/Tests/ITK/Modules/Core/TestKernel/include/itkTestingMacros.h:78:30: error: ‘Superclass’ has not been declared
   78 |   if (!std::strcmp((*object).Superclass::GetNameOfClass(), #SuperclassName))                                           \
      |                              ^~~~~~~~~~
/home/dzenan/Dashboards/Tests/ITK/Modules/Core/TestKernel/include/itkTestingMacros.h:78:30: note: in definition of macro ‘ITK_EXERCISE_BASIC_OBJECT_METHODS’
   78 |   if (!std::strcmp((*object).Superclass::GetNameOfClass(), #SuperclassName))                                           \
      |                              ^~~~~~~~~~

@thewtex
Copy link
Member Author

thewtex commented Sep 8, 2021

/azp run ITK.Windows

@jhlegarreta
Copy link
Member

Grasping at straws, but ... in, e.g., Modules/Nonunit/Review/include/itkLabelGeometryImageFilter.h, should itkTypeMacro be invoked before itkNewMacro? That might be helpful if, deep inside, itkNewMacro can print debug output that makes use of the GetNameOfClass method defined by itkTypeMacro. (Yeah, the compiler should be figuring this out, even if out of order, ....)

I was tempted to push a PR with that, but I found many counter-examples (classes whose RTTI macro declaration comes after the New macro and whose tests exercise the basic object methods, and the compiler seems not to complain), e.g.:
https://github.com/InsightSoftwareConsortium/ITK/blob/master/Modules/Core/Common/include/itkImportImageFilter.h#L65
https://github.com/InsightSoftwareConsortium/ITK/blob/master/Modules/Core/Common/test/itkImportImageTest.cxx#L44

Even in the Review module itself:
https://github.com/InsightSoftwareConsortium/ITK/blob/master/Modules/Nonunit/Review/include/itkAreaClosingImageFilter.h#L99
https://github.com/InsightSoftwareConsortium/ITK/blob/master/Modules/Nonunit/Review/test/itkAreaClosingImageFilterTest.cxx#L52

@thewtex thewtex force-pushed the label-geometry-superclass branch from a574d04 to c238b76 Compare September 9, 2021 12:27
@jhlegarreta jhlegarreta self-requested a review September 9, 2021 16:31
Copy link
Member

@jhlegarreta jhlegarreta left a comment

Choose a reason for hiding this comment

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

Not sure how to make the linter happy.

Thanks for doing this @thewtex.

@dzenanz
Copy link
Member

dzenanz commented Sep 9, 2021

If we can't make clang-format happy, there is /* clan-format: off */ (or something similar) to disable checking.

GCC is not resolving `object->Superclass` in some cases even though the
definition is available and other compilers are resolving the
definition.

xref InsightSoftwareConsortium#2676
@thewtex thewtex force-pushed the label-geometry-superclass branch from c238b76 to 230be6b Compare September 10, 2021 13:28
@jhlegarreta jhlegarreta merged commit d6b1224 into InsightSoftwareConsortium:master Sep 10, 2021
@thewtex thewtex deleted the label-geometry-superclass branch September 10, 2021 17:20
@dyollb
Copy link
Contributor

dyollb commented Jul 27, 2023

Maybe something like this would work:

#if defined(__GNUC__)
#define ITK_EXERCISE_BASIC_OBJECT_METHODS(object, ClassName, SuperclassName)                                           \
  object->Print(std::cout);                                                                                            \
  std::cout << "Name of Class = " << object->GetNameOfClass() << std::endl;                                            \
  using self_t = std::remove_reference<decltype(*object)>::type; \
  using parent_t = typename self_t::Superclass; \
  std::cout << "Name of Superclass = " << dynamic_cast<parent_t&>(*object).GetNameOfClass() << std::endl; \
  ITK_MACROEND_NOOP_STATEMENT

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

6 participants