Skip to content

Conversation

@nickrolfe
Copy link
Contributor

No description provided.

@nickrolfe nickrolfe added C++ depends on internal PR This PR should only be merged in sync with an internal Semmle PR labels Dec 6, 2018
@nickrolfe nickrolfe requested a review from a team as a code owner December 6, 2018 10:30
@adityasharad adityasharad added this to the 1.19 milestone Dec 6, 2018
Copy link
Contributor

@jbj jbj left a comment

Choose a reason for hiding this comment

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

It seems fine to me that a type argument to a template is normalised slightly to remove a pointless const. I suppose the same will now happen for a const int argument.

What happens if there's also a G<const int *> instantiation? Does that become a separate class but with the same template argument? Or will it be the same class as G<const int *const> and have two names? And how many bodies?

@nickrolfe
Copy link
Contributor Author

If you add a G<const int *> instantiation you do indeed get a separate class with the same template argument.

@jbj
Copy link
Contributor

jbj commented Dec 7, 2018

That's probably the least bad of the two possibilities.

Copy link
Contributor

@dave-bartolomeo dave-bartolomeo left a comment

Choose a reason for hiding this comment

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

Wait, what's the extractor change that causes this? G<const int*const> and G<const int*> are two different specializations of the template in C++. It's function parameters that ignore top-level const, not template arguments.

For a real-world example, std::unique_ptr<int> is an int* that gets deleted when it goes out of scope, and std::unique_ptr<const int> is a const int* that gets deleted when it goes out of scope.

@nickrolfe
Copy link
Contributor Author

This is not a desirable change, it's just an unfortunate side-effect of us struggling to work around an issue extracting instantiations like a<decltype(f)> - see internal PR 29618.

Copy link
Contributor

@dave-bartolomeo dave-bartolomeo left a comment

Choose a reason for hiding this comment

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

OK, it sounds like we're still keeping them as separate classes, but with top-level const incorrectly stripped from the template argument. That's still not good, but it's probably less likely to cause problems than having them as the same class. Is there a Jira ticket still open to figure out the right extractor fix post-1.19?

@dave-bartolomeo dave-bartolomeo dismissed their stale review December 7, 2018 18:27

Objection removed

@jbj
Copy link
Contributor

jbj commented Dec 10, 2018

Should this be closed now?

@nickrolfe
Copy link
Contributor Author

Yes, @ian-semmle figured out the correct extractor fix that keeps the correct output for this test.

@nickrolfe nickrolfe closed this Dec 10, 2018
@nickrolfe nickrolfe deleted the cpp214 branch December 10, 2018 22:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

C++ depends on internal PR This PR should only be merged in sync with an internal Semmle PR

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants