Skip to content

Conversation

iguessthislldo
Copy link
Member

@iguessthislldo iguessthislldo commented Dec 11, 2020

  • Added the following checks and changed OpenDDS to pass these checks:

    • whitespace_before_newline_in_string: ex: "This is a string \n"

    • missing_include_guard: Checks IDL and header files for include guards matching one of two patterns. Both start with the file path, take dds/ off the front, turn anything that's not alphanumeric or an underscore to underscores, uppercase everything and prepend OPENDDS_. The difference between them is one converts from camel case to snake case while the other doesn't. For example, dds/DdsDcps.idl can be OPENDDS_DDSDCPS_IDL or OPENDDS_DDS_DCPS_IDL.

    • nonrelative_include_path: Checks to see if an include statement path contains the including file's directory in it. For example if foo/bar.h has #include <foo/util/stuff.h>, it should be #include "util/stuff.h".

  • Added --try-fix to try to fix issues. So far this only works for the checks with strip_fix property set and the missing_include_guard check if there's an existing include guard.

  • Way to pass arguments to ACE fuzz.pl.

  • Add more info to --help message.

  • 🌈 ANSI Escape Code Colors in messages 🌈 Can be controlled using command line arguments and environment variables.

  • For checks using line_matched that support it, highlight what the offending characters of the line are in a separate error message.

  • Rename not_exactly_one_eof_newline check to eof_newline_count. Made it accept a blank line at the end of text files. Also fixed an issue where it actually wasn't catching when there were no newlines at the end of the file. This means there must be 1 or 2 newline characters at the end of a file now.

  • Checks are now listed and happen in consistent order now (alphabetically) and a proper error message is displayed when an invalid check is passed as an argument.

  • Ability to check paths outside of OpenDDS, assuming the check isn't OpenDDS specific.

- Added `whitespace_before_newline_in_string` and
  `missing_include_guard` checks.

- Added `--fix` to try to fix issues. So far this only works for the
  checks with `strip_fix` property set and the `missing_include_guard`
  check if there's an existing include guard.

- Way to pass arguments to ACE `fuzz.pl`.

- Add more info to `--help` message.

- ANSI Escape Code Colors in messages

- For checks using `line_matched` that support it, highlight what the
  offending characters of the line are in a separate error message.
Tabs usually are being used for indention, so have to be replace with
appropriate amount of spaces instead of just being removed.
Rename `not_exactly_one_eof_newline` check to `eof_newline_count`. Made
it accept a blank line at the end of text files. Also fixed an issue
where it actually wasn't catching when there were no newlines at the end
of the file. This means there must be 1 or two newline characters at the
end of a file now.
@jwillemsen
Copy link
Member

Running this kind of generic cleanup on the xtypes branch could result in more merge conflicts on master, wouldn't it be better to run this on master and merge that in xtypes to prevent merge conflicts?

@iguessthislldo
Copy link
Member Author

iguessthislldo commented Dec 11, 2020

I did consider that, but like in #2114, I'm trying to avoid putting these kinds of changes in master since we're trying to do a release. Plus doing it here means I can make sure the xtypes branch is compliant, instead of waiting until mater is merged in.

@iguessthislldo iguessthislldo added this to the OpenDDS 3.16 milestone Dec 11, 2020
Also:
- Ability to run checks on something that's not OpenDDS
- Proper error message if invalid check is passed
- Control color output with --[no-]color and NO_COLOR, Default is now
  based on isatty.
@iguessthislldo
Copy link
Member Author

I don't know what's going on in GCC6 yet, but the Java support has revealed an issue in tao_idl. tao_idl doesn't run the preprocessor on the IDL file itself, but on a temporary copy it creates with a cpp extension. I don't know how it is with msvc or other compilers, but it looks like it uses this to force gcc and clang to see the file as something to run the preprocessor on. Since it's in another location it breaks relative includes unless things happen to be set up correctly.

The way I see it there are at least three options:

  • Change tao_idl to stop creating a copy of the IDL file and use something like gcc -x c instead. I would like to do that because it seems like the proper thing to do, but I'm not sure how compilers other than clang will work with this.
  • Change tao_idl to add the directory of idl file to the include path. This is probably safer, however it will just simulate relative includes and won't work if trying to include staring with ...
  • Workaround it. Basically I would revert the IDL files changes for this and exclude IDL files from the nonrelative_include_path check. To be safe I'd do the same thing in Simplify Includes DOCGroup/ACE_TAO#1344. I'd prefer to avoid this.

iguessthislldo added a commit to iguessthislldo/ACE_TAO that referenced this pull request Dec 29, 2020
Fixes an issue that came up in
OpenDDS/OpenDDS#2161.

To make behavior of `#include ".."` consistent with C and C++, change
how `tao_idl` calls the C preprocessor by default in most cases from
making a copy of the IDL file to using the IDL file directly.
@iguessthislldo
Copy link
Member Author

The Java issue is fixed with ACE/TAO #1357.

iguessthislldo added a commit to iguessthislldo/ACE_TAO that referenced this pull request Dec 30, 2020
This is a backport of the same commit in
DOCGroup#1357, but with copy
preprocessor input method being the default.

Fixes an issue that came up in
OpenDDS/OpenDDS#2161.

To make behavior of `#include ".."` consistent with C and C++, change
how `tao_idl` calls the C preprocessor by default in most cases from
making a copy of the IDL file to using the IDL file directly.
@mitza-oci mitza-oci closed this Jan 17, 2021
@mitza-oci mitza-oci deleted the branch OpenDDS:xtypes January 17, 2021 23:34
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.

3 participants