cyclonedds-cxx icon indicating copy to clipboard operation
cyclonedds-cxx copied to clipboard

IDLCXX generated code doesn't use unique include guards

Open sumanth-nirmal opened this issue 4 years ago • 4 comments

Required information

Operating system: Ubuntu 18.04 LTS

Cyclone Version

  • cyclonedds - master branch, https://github.com/eclipse-cyclonedds/cyclonedds/commit/9e0d5bc69f83795e0b0855c2e4afdeecfd6b687c
  • cyclonedds-cxx - template_streaming , https://github.com/eclipse-cyclonedds/cyclonedds-cxx/commit/39ffc05dd419ecd661c8f4d090289b4fc6ed9f5f

Observed result or behaviour:

There is an issue with include guards in the generated code, The following scenario should explain the problem.
If we have the following IDL files, with the same name but in different paths

  1. Duration.idl in path foo1/bar1/msg/
  2. Duration.idl in path foo2/bar2/msg/

The generated header for both the IDL's has the same include guard as below

#ifndef DDSCXX_DURATION__HPP
#define DDSCXX_DURATION__HPP
...
#endif /* DDSCXX_DURATION__HPP */

If the user code wants to use both the Duration.idls above and include them in the code as below

#include <foo1/bar1/msg/Duration.hpp>
#include <foo2/bar2/msg/Duration.hpp>
...

because of the include guards, it will only find the definitions for one file, and can't find the definitions for the other!

Expected result or behaviour:

The include guards for the generated headers should be unique. It can probably also take the namespace into account instead of just using the name of the IDL, or use some hash of the file contents along with the name of the IDL.

sumanth-nirmal avatar May 15 '21 01:05 sumanth-nirmal

@sumanth-nirmal, since we're using the path of the header, I'm hoping this is resolved if we implement the functionality from #947 for C++ too. Or rather, actually make sure the relative path is retained :sweat_smile:. That it doesn't work right now is a bug. Of course, feel free to comment if that doesn't work for you or if you feel there's issues with that approach.

k0ekk0ek avatar Sep 23 '21 09:09 k0ekk0ek

I just ran into this as well. Was #pragma once considered instead of header guards? It would be an easy fix, but we would lose the ability to check what's included with #ifdef DDSCXX_DURATION_HPP.

ghorn avatar Apr 26 '22 00:04 ghorn

I don't think it was considered, but it seems to be quite well supported (https://en.wikipedia.org/wiki/Pragma_once#Portability) and perhaps it ought to be. @reicheratwork you know the C++ backend best, would this be a sensible thing to do?

eboasson avatar Apr 26 '22 07:04 eboasson

@eboasson: using #pragma once is the most "C++" way to do this, though I a personally not that familiar with CMake to be able to say whether that does not produce weird side-effects due to CMake moving files around etc.

If it is just the requirement to have a unique enough #ifdef for each file so that we can be reasonably certain that there won't be any collisions, even in the case of files having the same names, maybe one solution is to just postfix each #ifdef statement with (a part of) an hash (MD5?) of the contents inside the statement. This would have the advantage that if the generated files result in the same code, then the include guards are also identical. The disadvantage of this approach is that it makes it more difficult to check for certain include statements to see whether files have been included or not.

reicheratwork avatar Apr 26 '22 09:04 reicheratwork