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

tao_idl Improvements #1357

Draft
wants to merge 10 commits into
base: master
Choose a base branch
from

Conversation

iguessthislldo
Copy link
Member

@iguessthislldo iguessthislldo commented Dec 29, 2020

  • Made IDL 4 the default.
  • Added an IDL preprocessor macro for the IDL version, __TAO_IDL_IDL_VERSION.
  • Added proper documentation for the __TAO_IDL IDL preprocessor macro.
  • Remove support for long deprecated CPP_LOCATION and TAO_IDL_DEFAULT_CPP_FLAGS environment variables.
  • Fixed issue encountered in this OpenDDS PR where OpenDDS' idl2jni, which is a tao_idl-derived compiler, can't properly handle relative includes because tao_idl makes a temporary copy that is passed to the preprocessor. Fixed this by making it preferred to pass the IDL file to the preprocessor directly.

Remove support for these environment variables because they have been
marked as deprecated for at least 20 years at this point.
Use conventional HTML, non breaking dashs, remove some outdated
information.
Make it easier to read and edit by replacing tabs with spaces.
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.
@jwillemsen
Copy link
Member

Very likely some old preprocessors had issues when passing them a IDL file directly due to the idl extension, some required it to be c/cpp

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.
else
{
i++;
if (!ACE_OS::strcmp (av[i], "guess"))
Copy link
Member

Choose a reason for hiding this comment

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

Can use std::strcmp now without problems

ACE_DEBUG ((LM_DEBUG, "%C: guessing preprocessor input method using name: %C\n",
idl_global->prog_name (), cpp_name));
}
if (ACE_OS::strstr (cpp_name, "g++")) // g++ or clang++
Copy link
Member

Choose a reason for hiding this comment

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

can use std::strstr

// method. Warn about it anyway, because it shouldn't happen.
idl_global->preprocessor_input_ = IDL_GlobalData::PreprocessorInputCopy;
if (!(idl_global->compile_flags () & IDL_CF_NOWARNINGS))
ACE_ERROR ((LM_WARNING,
Copy link
Member

Choose a reason for hiding this comment

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

ACE_DEBUG?

// Decide how files will be passed to the preprocessor
if (idl_global->preprocessor_input_ == IDL_GlobalData::PreprocessorInputGuess)
{
const char * const cpp_name = tolower (our_basename (FE_get_cpp_loc_from_env ()));
Copy link
Member

@jwillemsen jwillemsen Jan 1, 2021

Choose a reason for hiding this comment

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

What about setting the default through the various config-* files, there we also set the default preprocessor to be used? We have there ACE_CC_PREPROCESSOR and ACE_CC_PREPROCESSOR_ARGS already. We could add a define there to set the preprocessor input handling type. That looks to be easier than trying to determine it here based on the name of the pre processor separately here. When we set the default through a define the user can always overrule it with a different setting on the commandline

@jwillemsen
Copy link
Member

Found recently that bcc32c/bcc64 (the Embarcadero clang based compilers) can't preprocess a file with a IDL extension, we need a copy with cpp extension with those compilers.

@iguessthislldo iguessthislldo marked this pull request as draft June 9, 2024 07:00
@iguessthislldo
Copy link
Member Author

Found recently that bcc32c/bcc64 (the Embarcadero clang based compilers) can't preprocess a file with a IDL extension, we need a copy with cpp extension with those compilers.

I know it's been a long time, but are you sure it can't do it like normal clang using -x c++ -E? If it's clang-based, it seems strange to remove features.

@jwillemsen
Copy link
Member

bcc64/bcc64x seems to support -x c++ -E but bcc32c (which is also clang based) doesn't support -E, there is an explicit Embarcadero message that -E isn't supported Warning: option '-E' is not supported in Clang-based compiler. and there is no output

@iguessthislldo
Copy link
Member Author

So I decided to download it to see what it's doing (this was annoying because they wanted an account, but didn't even give me the 64-bit compilers...). bcc32x -x c++ -E works and that's the one being used by tao_idl. So if bcc64 also works, then I think it can use the GCC/Clang direct method without an issue.

@iguessthislldo
Copy link
Member Author

To give the current context, I'm trying to see what it would take to get this PR merged because I occasionally see tao_idl failing because of the temporary file on Windows: D:/a/OpenDDS/OpenDDS/OpenDDS/build/bin/Release/opendds_idl.exe: Unable to rename temporary file "C:\Users\RUNNER~1\AppData\Local\Temp\tao-idlf_pTNkVl" to "C:\Users\RUNNER~1\AppData\Local\Temp\tao-idlf_pTNkVl.cpp": Input/output error

Log link

This is probably a problem with GitHubs VMs, but I wanted to do this anyways.

I also noticed it just failed in a job from the merge commit: D:\a\ACE_TAO\ACE_TAO/ACE\bin\tao_idl: Unable to rename temporary file "C:\Users\RUNNER~1\AppData\Local\Temp\tao-idlf_KzZPyr" to "C:\Users\RUNNER~1\AppData\Local\Temp\tao-idlf_KzZPyr.cpp": Input/output error

Log link

However because this is in this PR, then something's wrong with my current code and it shouldn't have done this. So the guessing probably isn't working on Windows. As Johnny suggested originally I think I'm going to have it work use macros, at least for the defaults, but I need to think about the specifics.

@jwillemsen
Copy link
Member

Would be nice when we don't need that copy step anymore with most compilers, will speed up the builds a little bit, especially on Windows

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

Successfully merging this pull request may close these issues.

2 participants