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

Remove all redundant header guards around #include directives #341

Closed
wants to merge 3 commits into from

Conversation

Teemperor
Copy link
Collaborator

@Teemperor Teemperor commented Feb 10, 2017

Some headers contain redundant header guards around #include directives like this:

#ifndef ROOT_TTree
#include "TTree.h"
#endif

This patch removes the #ifndef's around these includes as they don't serve any practical purpose and are no longer part of the current ROOT coding convention.

This patch also fixes the 153 typos that are contained in the symbols of the #ifndef directives.

@phsft-bot
Copy link
Collaborator

Can one of the admins verify this patch?

@Teemperor
Copy link
Collaborator Author

Teemperor commented Feb 10, 2017

A more detailed report for the changes in this patch is here: https://gist.github.com/Teemperor/480b4105a31ce09409a162c9ee66003b

The changes are done by a script that compares the header guards around the include with the header guards in the respective header. It also does some typo detection.

In some cases the script isn't sure if the typo correction works correctly and didn't do any changes. It also ignored a few system headers. See the report for a list of those cases.

In the report every handled header guard is listed in the form
FILE
FOUND HEADER GUARD SYMBOL
HEADER GUARD USED IN THE IFNDEF

The script is available here: https://gist.github.com/Teemperor/767cbd5ec74a3c6444d9548414150c0a

@Axel-Naumann
Copy link
Member

@phsft-bot build!

@pcanal
Copy link
Member

pcanal commented Feb 10, 2017

Could add a commit (and/or another merge request) with the propose typo fixes?

@Axel-Naumann
Copy link
Member

Axel-Naumann commented Feb 10, 2017 via email

@@ -16,6 +13,8 @@
#define ROOT_TGLContext

#include <memory>
#include <utility>
#include <list>
Copy link
Member

Choose a reason for hiding this comment

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

Could you order the includes alphabetically?

@vgvassilev
Copy link
Member

@phsft-bot build!

@pcanal
Copy link
Member

pcanal commented Feb 13, 2017

One example that is listed in

https://gist.github.com/Teemperor/480b4105a31ce09409a162c9ee66003b

but is not yet in the PR seems to be:

./io/io/inc/TMakeProject.h:
ROOT_TString
ROOT_TOBJECT_H

This what I expected since this is listed under "Cases the script can't handle "

@Teemperor
Copy link
Collaborator Author

Here is the diff with context of the things that were NOT done because the script couldn't handle them: http://pastebin.com/tRkBdfaM

@Teemperor
Copy link
Collaborator Author

Here is a diff of things we did do, but where the script isn't 100% sure: http://pastebin.com/CXtS5EQj

@vgvassilev
Copy link
Member

@pcanal, is there a reason not to rely on a post commit review here?

@pcanal
Copy link
Member

pcanal commented Feb 13, 2017

All except for:

 -#ifndef MATH_NO_PLUGIN_MANAGER
 #include "TEnv.h"
-#endif

are indeed typos. The one above is to avoid including an unnecessary header when a feature is turned off.

Can you update the PR to remove all (but MATH_NO_PLUGIN_MANAGER) cases? (and then I will upload).

Thanks,
Philippe.

@pcanal
Copy link
Member

pcanal commented Feb 13, 2017

@pcanal, is there a reason not to rely on a post commit review here?

Yes. In general, in my opinion, we should move towards more pre-commit review than post-commit review ... and in this case, the pre-commit review (as far as I am concerned) is done.

@vgvassilev
Copy link
Member

Ok, good. @Teemperor, please update the patch and I will check it in.

Many headers contains redundant header guards around #include directives:

    #ifndef ROOT_TTree
    #include "TTree.h"
    #endif

This patch removes the #ifndef's around these includes as they
don't serve any pratical purpose anymore and are no longer part
of the current ROOT coding convention.

This patch also fixes the 153 typos that are contained in the
symbols of the #ifndef directives.
The script didn't catch these in the previouis commit, so we
had to manually do this.
@Teemperor
Copy link
Collaborator Author

@vgvassilev done

@vgvassilev
Copy link
Member

@phsft-bot build!

@vgvassilev
Copy link
Member

Merged. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants