open-dis / open-dis-cpp

C++ implementation of the IEEE-1278.1 Distributed Interactive Simulation (DIS) application protocol v6 and v7
BSD 2-Clause "Simplified" License
91 stars 66 forks source link

include guards should contain namespace #51

Open phoppermann opened 3 years ago

phoppermann commented 3 years ago

to avoid potential name clashes

e.g. in https://github.com/open-dis/open-dis-cpp/blob/a9f9f26cb5eff47235559135f39c47ac57a82add/src/dis6/AcknowledgePdu.h#L2

ACKNOWLEDGEPDU_H to DIS_ACKNOWLEDGEPDU_H

alternatively: use #pragma once

rodneyp290 commented 3 years ago

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 commented 3 years ago

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

This should be implemented in xmlpg as well.

leif81 commented 3 years ago

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 commented 3 years ago

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

rodneyp290 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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