[SofaKernel] Header include cleanup #638

epernod · 2018-04-16T09:34:26Z

Just removing unnecessary headers include inside header in sofa::core
No forward class or intelligent refactoring.

any suggestion/remark is welcomed.

This PR:

builds with SUCCESS for all platforms on the CI.
does not generate new warnings.
does not generate new unit test failures.
does not generate new scene test failures.
does not break API compatibility.
is more than 1 week old (or has fast-merge label).

Reviewers will merge only if all these checks are true.

C2057: expected constant expression

…template is used. This fix error C2057 on stl container.

…ide sofaBaseCollision headers.

…till needed.

…BaseVisual and SofaBaseTopology.

…now in application_SofaGuiGlut

…er headers. Remove GeneralLoader.h and GeneralLoader.cpp which are not anymore used.

damienmarchal · 2018-05-15T08:38:33Z

[ci-build][with-scene-tests]

damienmarchal · 2018-05-15T09:31:10Z

modules/SofaGeneralLoader/SphereLoader.h

@@ -21,11 +21,8 @@
 ******************************************************************************/
 #ifndef SOFA_COMPONENT_LOADER_SPHERELOADER_H
 #define SOFA_COMPONENT_LOADER_SPHERELOADER_H
-#include "config.h"


I'm not sure about removing this one....because no one will define SOFA_TARGET anymore.

While it is supposed to be used in the ObjectFactory:

/// The name of the library or executable containing the binary code for this component virtual const char* getTarget() { #ifdef SOFA_TARGET return sofa_tostring(SOFA_TARGET); #else return ""; #endif }

EDIT: Actually all the component in SofaGeneralLoader should have somewhere the target set to SOFA_TARGET "SofaGeneralLoader" (which I'm sure is what is happening).

NB: I'm really not found of silently ignoring the undefined SOFA_TARGET.

ok I can revert that. I think it has been removed because this class include already another loader header that include config.h
If the logic is: every class include it's config.h, I'm ok with it.

Maybe we should replace this code by:

#ifndef SOFA_TARGET #error....the SOFA_TARGET is not defined. #else: return sofa_tostring(SOFA_TARGET); #endif

(Not in this PR of course)

I will put those headers back and we will see later where those target should be set.

damienmarchal · 2018-05-15T09:34:34Z

modules/SofaGeneralLoader/GeneralLoader.h

@@ -1,33 +0,0 @@
-/******************************************************************************


Hello Erik,

I'm not sure this one should be removed, on the contrary its use should be generalized. Additionally I think it should also define the SOFA_TARGET to SofaGeneralLoader.

Please confirm @guparan

Ok I sent a message to @guparan already regarding this.
Right now all macros are defined in sofa-build\include\SofaMisc\config.h etc..
This class is not included. Let me know which one should be kept. I'll revert the commit if needed.

I will report our discussion in an issue and remove that change from the PR to have only changes on header includes.

damienmarchal · 2018-05-15T09:35:28Z

modules/SofaGeneralLoader/GeneralLoader.cpp

@@ -1,35 +0,0 @@
-/******************************************************************************


Why did you removed also code ?

From PR description I would have expected only changing lines with #include

yes, I saw all includes could be removed because this class is not anymore in the cmakeLists.
Another PR regarding deprecated files will be better indeed because I found other similar cases.

…a::core::topology

…fa::module::topology

…:behavior

…:loader

…:objectmodel

…r::logging

…:visual . Had to put back include of gl.h in several components.

…that has no include at all....

…ncludes.

epernod · 2018-05-28T20:21:33Z

[ci-build][with-scene-tests]

damienmarchal · 2018-05-31T07:34:30Z

READY and recently compiled over the issofa PR ...I merge it.

damienmarchal · 2018-05-31T08:25:46Z

SofaKernel/framework/sofa/core/behavior/BaseMechanicalState.h

@@ -22,17 +22,14 @@
 #ifndef SOFA_CORE_BEHAVIOR_BASEMECHANICALSTATE_H
 #define SOFA_CORE_BEHAVIOR_BASEMECHANICALSTATE_H

-#include <sofa/config/build_option_experimental_features.h>


I merged without noticing this one.
It shouldnt be removed.
Because this one add some macro that are tested with
#ifdef XXXXX

So removing it "compile" but the resulting code is not working anymore.
....

A very bad things is that even in condition like #if MACRO == 1
When the macro is not defined then its value is == 0 so removing the include line is silent.
https://gcc.gnu.org/onlinedocs/cpp/If.html

A workaround I can propose it to do:

#define MYTEST() 0 .... #if MYTEST() std::cout << "COUCOU A" << std::endl ; #else std::cout << "COUCOU B" << std::endl ; #endif

So that we get error when MYTEST is not defined (the bad things is that the error is not very clear with some compilers):
with gcc:
toto.cpp:8:11: error: missing binary operator before token "("
#if MYTEST()

with clang:
toto.cpp:8:5: error: function-like macro 'MYTEST' is not defined
#if MYTEST()
^

guparan and others added 4 commits April 10, 2018 14:31

[all] FIX VS2017 build (Windows)

b7b5d9b

C2057: expected constant expression

FIX: VS2017 another way: explicit template instantiations before the …

47ae31d

…template is used. This fix error C2057 on stl container.

Merge remote-tracking branch 'upstream/master'

ea5921a

[SofaBaseCollision] UPDATE: remove all unnecessary header include ins…

870287f

…ide sofaBaseCollision headers.

epernod added the pr: status wip Development in the pull-request is still in progress label Apr 16, 2018

epernod changed the title ~~Header include cleanup~~ [SofaKernel] Header include cleanup Apr 17, 2018

[SofaBaseCollision] FIX: compilation. BaseIntTool header include is s…

2e1c309

…till needed.

epernod self-assigned this Apr 18, 2018

[SofaKernel] remove some header includes inside other headers in Sofa…

d07bff6

…BaseVisual and SofaBaseTopology.

hugtalbot added the pr: clean Cleaning the code label Apr 24, 2018

epernod added this to the v18.06 milestone May 9, 2018

epernod added 2 commits May 10, 2018 15:42

[SofaGui CMakeLists] Remove deprecated option SOFA_GUI_GLUT which is …

b3c87d3

…now in application_SofaGuiGlut

[SofaGeneralLoader] CLEAN: remove unnecessary header includes in load…

2cefc62

…er headers. Remove GeneralLoader.h and GeneralLoader.cpp which are not anymore used.

damienmarchal reviewed May 15, 2018

View reviewed changes

epernod added 13 commits May 15, 2018 15:09

[SofaKernelTopology] CLEAN: remove unnecessary header includes in sof…

96c2049

…a::core::topology

[SofaGeneralTopology] CLEAN: remove unnecessary header includes in so…

bd51340

…fa::module::topology

[SofaKernel] CLEAN: remove unnecessary header includes in sofa::core

738b61c

[SofaKernel] CLEAN: remove unnecessary header includes in sofa::core:…

aac50b2

…:behavior

[SofaKernel] CLEAN: remove unnecessary header includes in sofa::core:…

a8b27ec

…:loader

[SofaKernel] FIX: compilation on gcc

847bff6

[SofaKernel] CLEAN: remove unnecessary header includes in sofa::core:…

00fc74c

…:objectmodel

[SofaKernel] CLEAN: remove unnecessary header includes in sofa::helpe…

c383555

…r::logging

[SofaKernel] FIX: compilation

4fe1fa2

[SofaKernel] FIX: Try to fix linux compilation

641378a

[SofaKernel] FIX: Try to fix macos compilation

d553c2b

[SofaKernel] FIX: Try to fix msvc and macos compilations

4ab963c

[SofaKernel] FIX: Try to fix macos compilation again

9a65c7f

epernod added 8 commits May 23, 2018 14:25

[SofaKernel] CLEAN: remove unnecessary header includes in sofa::core:…

64854e7

…:visual . Had to put back include of gl.h in several components.

[Plugin Registration] Fix compilation due to header move.

09b1958

[Plugin Flexible] Fix compilation due to header move. in an inl file …

f5e7a87

…that has no include at all....

[Plugin optiTrackNatNet] Fix compilation due to header move.

940555e

[SofaKernel] FIX: compilation, strlen is in cstring not string.

e5598fb

[SofaKernel] FIX: reverse last commit

0826474

[SofaKernel] FIX: compilation of StringUtils on linux

e2cf12b

[SofaGeneralLoader] Revert: class GeneralLoader.h and some config.h i…

b5584a2

…ncludes.

epernod added pr: status to review To notify reviewers to review this pull-request and removed pr: status wip Development in the pull-request is still in progress labels May 28, 2018

hugtalbot added pr: status ready Approved a pull-request, ready to be squashed and removed pr: status to review To notify reviewers to review this pull-request labels May 30, 2018

Merge branch 'master' into header_include_cleanup

bb4e3d2

damienmarchal merged commit e959abb into sofa-framework:master May 31, 2018

epernod deleted the header_include_cleanup branch May 31, 2018 07:53

damienmarchal reviewed May 31, 2018

View reviewed changes

damienmarchal mentioned this pull request May 31, 2018

[SofaKernel] FIX macro issue resulted from the #include cleaning. #672

Merged

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SofaKernel] Header include cleanup #638

[SofaKernel] Header include cleanup #638

epernod commented Apr 16, 2018 •

edited

Loading

damienmarchal commented May 15, 2018

damienmarchal May 15, 2018 •

edited

Loading

epernod May 15, 2018

damienmarchal May 18, 2018

epernod May 23, 2018 •

edited

Loading

damienmarchal May 15, 2018

epernod May 15, 2018

epernod May 23, 2018

damienmarchal May 15, 2018 •

edited

Loading

epernod May 15, 2018

epernod commented May 28, 2018

damienmarchal commented May 31, 2018

damienmarchal May 31, 2018 •

edited

Loading

damienmarchal May 31, 2018

		@@ -1,33 +0,0 @@
		/******************************************************************************

		@@ -1,35 +0,0 @@
		/******************************************************************************

[SofaKernel] Header include cleanup #638

[SofaKernel] Header include cleanup #638

Conversation

epernod commented Apr 16, 2018 • edited Loading

damienmarchal commented May 15, 2018

damienmarchal May 15, 2018 • edited Loading

Choose a reason for hiding this comment

epernod May 15, 2018

Choose a reason for hiding this comment

damienmarchal May 18, 2018

Choose a reason for hiding this comment

epernod May 23, 2018 • edited Loading

Choose a reason for hiding this comment

damienmarchal May 15, 2018

Choose a reason for hiding this comment

epernod May 15, 2018

Choose a reason for hiding this comment

epernod May 23, 2018

Choose a reason for hiding this comment

damienmarchal May 15, 2018 • edited Loading

Choose a reason for hiding this comment

epernod May 15, 2018

Choose a reason for hiding this comment

epernod commented May 28, 2018

damienmarchal commented May 31, 2018

damienmarchal May 31, 2018 • edited Loading

Choose a reason for hiding this comment

damienmarchal May 31, 2018

Choose a reason for hiding this comment

epernod commented Apr 16, 2018 •

edited

Loading

damienmarchal May 15, 2018 •

edited

Loading

epernod May 23, 2018 •

edited

Loading

damienmarchal May 15, 2018 •

edited

Loading

damienmarchal May 31, 2018 •

edited

Loading