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

include guards should contain namespace #51

Open
phoppermann opened this issue Mar 16, 2021 · 8 comments
Open

include guards should contain namespace #51

phoppermann opened this issue Mar 16, 2021 · 8 comments

Comments

@phoppermann
Copy link
Contributor

to avoid potential name clashes

e.g. in

#define ACKNOWLEDGEPDU_H

ACKNOWLEDGEPDU_H to DIS_ACKNOWLEDGEPDU_H

alternatively: use #pragma once

@rodneyp290
Copy link
Contributor

I wrote this script which seems to make the right changes, but thought it might be best to wait for the current PRs to be resolved first, committing and making a PR. Putting here for safe keeping, and further discussion.

Although maybe the #pragma once is worth a look, I always thought that was an exclusive MSVC++ thing, but apparently it is supported by other compilers

(script to be run in root of repo)

pushd src/dis6/
for hf in *.h; do
  sed -Ei  's/^(#(ifndef|define|endif\s+\/\/)\s+)([A-Z_]+_(H|h))/\1OPENDIS6_\3/' $hf;
done
popd
pushd src/dis7/
for hf in *.h; do
  sed -Ei  's/^(#(ifndef|define|endif\s+\/\/)\s+)([A-Z_]+_(H|h))/\1OPENDIS7_\3/' $hf;
done
popd
pushd src/utils/
for hf in *.h; do
  sed -Ei  's/^(#(ifndef|define|endif\s+\/\/)\s+)([A-Z_]+_(H|h))/\1OPENDIS_UTILS_\3/' $hf;
done
popd

@phoppermann
Copy link
Contributor Author

#pragma once would be fine for me as well. @leif81 what do you think?

This should be implemented in xmlpg as well.

@leif81
Copy link
Member

leif81 commented Apr 15, 2021

#pragma once looks like it has very good compiler support now so that seems like a good option to me too. Feel free to submit PR for that.

@phoppermann
Copy link
Contributor Author

@rodneyp290 could you change your script to use #pragma once?

@rodneyp290
Copy link
Contributor

rodneyp290 commented Apr 20, 2021

Took a little bit of fiddling to get it right, but I think this replaces and delete all the necessary lines.
I realised the the previous script missed a few files (Vector3 type files), so I hope this isn't missing anything.
But it was still able to compile after running the script so I think it is good.

pushd src/dis6/
for hf in *.h; do
  sed -Ei  's/^#ifndef\s+[A-Z0-9_]+_(H|h)/#pragma once/' $hf;
  sed -Ei  '/^#define\s+(\/\/\s+|)[A-Z_]+_(H|h)/d' $hf;
  sed -Ei  '/^#endif/d' $hf;
done
popd
pushd src/dis7/
for hf in *.h; do
  sed -Ei  's/^#ifndef\s+[A-Z0-9_]+_(H|h)/#pragma once/' $hf;
  sed -Ei  '/^#define\s+(\/\/\s+|)[A-Z_]+_(H|h)/d' $hf;
  sed -Ei  '/^#endif/d' $hf;
done
popd
pushd src/utils/
for hf in *.h; do
  sed -Ei  's/^#ifndef\s+[A-Z0-9_]+_(H|h)/#pragma once/' $hf;
  sed -Ei  '/^#(define|endif)\s+(\/\/\s+|)[A-Z_]+_(H|h|)/d' $hf;
done
popd

This doesn't include ./src/common/msLibMacro.h, but I think a manual edit will be better there.

@leif81
Copy link
Member

leif81 commented Apr 20, 2021

I'm on "vacation" this week away from my PC. Can you run the script and submit the change as a PR? I'm available to click merge.

@rodneyp290
Copy link
Contributor

Turns out I had @kurtsansom's #44 PR checked out when writing the script, so I'd prefer to wait until that is completed, and then merged.
But I might try rewrite it and apply it over the weekend, and then test locally if the branches will merge nicely.

@rodneyp290
Copy link
Contributor

Turns out I underestimated git merge, as it merged almost flawlessly.
There was a merge conflict merging in #44 because I had changed the guard on src/dis7/msLibMacro.h, which is removed by #44.

So I deciding to leave that file with include guards, so that when #44 merges with the click of a button.
I even tested a merge with @kurtsansom's current wip branch and that worked too.
All merges compiled & successfully run the Example Programs.

PR incoming

rodneyp290 added a commit to rodneyp290/open-dis-cpp that referenced this issue Apr 25, 2021
leif81 added a commit that referenced this issue Apr 25, 2021
Replace Include Guards with #pragma once (Issue #51)
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

No branches or pull requests

3 participants