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

Temporarily suppress dependent name types #329

Merged
merged 9 commits into from
Jun 6, 2020
Merged

Conversation

jslee02
Copy link
Member

@jslee02 jslee02 commented May 5, 2020

Related issue: #328

@jslee02 jslee02 added this to the Chimera 0.2.0 milestone May 5, 2020
@jslee02 jslee02 requested a review from psigen May 5, 2020 03:58
Copy link
Member

@psigen psigen left a comment

Choose a reason for hiding this comment

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

Before we suppress this, would it be possible to trace which string function is returning the unspecified template type instead of the fully-qualified template type?

Perhaps we can fix the issue instead of concealing it?


def test_issue328(self):
self._test_issue328(py11)
self._test_issue328(boost)
Copy link
Member

@psigen psigen May 5, 2020

Choose a reason for hiding this comment

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

In situations where we are just suppressing an issue instead of solving it, could we add an assertion that should work if we didn't have the error (like being able to access this ::type) and then comment it out in the test?

I am worried it will be hard to tell which issues we have solved and which ones we have just covered up. This makes it hard if we e.g. upgrade clang: some of this functionality might get fixed, but we won't know how to test if it is without reading many old bug reports.

@jslee02
Copy link
Member Author

jslee02 commented May 5, 2020

Before we suppress this, would it be possible to trace which string function is returning the unspecified template type instead of the fully-qualified template type?

I fully agree with you. Sorry, I should have provided more details.

The intention is actually resolving the issue in the end instead of just concealing it. Supporting the built-in type (#327) is in the path for example. This PR is just to suppress throwing the exception for now. I think we should print a warning saying that the API is ignored as it's not supported yet in the meantime.

At a high level, I would like to extend the chimera features to reasonably support C++ projects with lots of templates. As an example, I tried to use chimera for FCL 0.6, and this segfault the first error I got. 😞

@jslee02 jslee02 changed the title Ignore dependent name types Temporarily suppress dependent name types May 5, 2020
@psigen
Copy link
Member

psigen commented May 11, 2020

Ok, that makes sense. Perhaps just add that placeholder unit test that shows the problem as per https://github.com/personalrobotics/chimera/pull/329/files#r419865135 and it should be fine to merge.

Copy link
Member

@psigen psigen left a comment

Choose a reason for hiding this comment

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

Looks good now.

@jslee02 jslee02 merged commit 4f88ae8 into master Jun 6, 2020
@jslee02 jslee02 deleted the ignore_dependent_name branch June 6, 2020 03:41
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.

2 participants