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

Fix hash_template_argument_from_pretty_func for GCC 9 #13449

Merged
merged 1 commit into from
May 31, 2020
Merged

Fix hash_template_argument_from_pretty_func for GCC 9 #13449

merged 1 commit into from
May 31, 2020

Conversation

jamiesnape
Copy link
Contributor

@jamiesnape jamiesnape commented May 29, 2020

Relates #13102.

__PRETTY_FUNCTION__ changed between GCC 7 and 9 as follows:

  • GCC 7:
    drake::test::{anonymous}::NiceAnonEnumTemplate<(drake::test::<unnamed>::AnonEnum)0>
  • GCC 9:
    drake::test::{anonymous}::NiceAnonEnumTemplate<drake::test::<unnamed>::AnonEnum::kFoo>
    

So it similar to Clang modulo {anonymous} and <unnamed> both mapping to (anonymous namespace):

drake::test::(anonymous namespace)::NiceAnonEnumTemplate<drake::test::(anonymous namespace)::AnonEnum::kFoo>

This therefore means that the drake::test::<unnamed>::AnonEnum is no longer discarded since it is not parenthesized and the difference between <unnamed> and {anonymous} matters, not least because the (unexpected) <> causes hash_template_argument_from_pretty_func to fail.


This change is Reviewable

@jamiesnape
Copy link
Contributor Author

@drake-jenkins-bot linux-focal-unprovisioned-gcc-bazel-experimental-release please

@jamiesnape
Copy link
Contributor Author

+@jwnimmer-tri for feature review.

Copy link
Collaborator

@jwnimmer-tri jwnimmer-tri left a comment

Choose a reason for hiding this comment

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

:lgtm: feature.

+@sherm1 for platform review per schedule, please.

Reviewed 2 of 2 files at r1.
Reviewable status: LGTM missing from assignee sherm1(platform) (waiting on @sherm1)

Copy link
Member

@sherm1 sherm1 left a comment

Choose a reason for hiding this comment

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

Platform :lgtm: with one suggestion.

Reviewed 2 of 2 files at r1.
Reviewable status: 1 unresolved discussion (waiting on @jamiesnape)


common/value.h, line 344 at r1 (raw file):

      }
    }
    if (*unnamed_iter == 0) {

minor: does this comparison have to be written bytewise? Consider instead:

if (strcmp(p, unnamed_spelling) == 0) {
  // ... output {anonymous} ...
  p += 9;  // Length of "<unnamed>".
}

That would be easier to read (assuming it works).

Copy link
Collaborator

@jwnimmer-tri jwnimmer-tri left a comment

Choose a reason for hiding this comment

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

Reviewable status: :shipit: complete! all discussions resolved, LGTM from assignees jwnimmer-tri(platform),sherm1(platform)


common/value.h, line 344 at r1 (raw file):

Previously, sherm1 (Michael Sherman) wrote…

minor: does this comparison have to be written bytewise? Consider instead:

if (strcmp(p, unnamed_spelling) == 0) {
  // ... output {anonymous} ...
  p += 9;  // Length of "<unnamed>".
}

That would be easier to read (assuming it works).

strcmp is not constexpr
https://en.cppreference.com/w/c/string/byte/strcmp

@jwnimmer-tri jwnimmer-tri merged commit 8c194d5 into RobotLocomotion:master May 31, 2020
@jamiesnape jamiesnape deleted the common-value-test-gcc-9 branch February 1, 2021 15:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: build system Bazel, CMake, dependencies, memory checkers, linters priority: low unused team: kitware
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants