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

Enable debug info by default #4232

Merged
merged 7 commits into from
Aug 22, 2024

Conversation

dwblaikie
Copy link
Contributor

@dwblaikie dwblaikie commented Aug 21, 2024

Discussed in the toolchain meeting today - we'd like to try having this on by default and see if the cost isn't too high.

The nodebug test is a bit verbose, because it doesn't have the --exclude-dump-file-prefix that test_file would usually add. Is there a nicer way I could write this test to verify that --no-debug-info does what it's meant to?

Discussed in the toolchain meeting today - we'd like to try having this
on by default and see if the cost isn't too high.

The nodebug test is a bit verbose, because it doesn't have the
`--exclude-dump-file-prefix` that test_file would usually add. Is there
a nicer way I could write this test to verify that --no-debug-info does
what it's meant to?
@jonmeow
Copy link
Contributor

jonmeow commented Aug 21, 2024

Note, I removed the description boilerplate from the top comment.

Note nodebug isn't just verbose, it includes your local machine paths; it's host-dependent. Maybe add something like %{core_package_dir} to file_test support so that you can add --exclude-dump-file-prefix=%{core_package_dir} to ARGS? (it seems fine if you want to leave that as a TODO, but the test file would then need to be removed from the PR)

@dwblaikie
Copy link
Contributor Author

Note, I removed the description boilerplate from the top comment.

Ah, thanks for that! I'll figure out the description template, etc eventually/keep an eye out for that in the future.

Note nodebug isn't just verbose, it includes your local machine paths; it's host-dependent.

Oh, right - good call/catch. Thanks!

Maybe add something like %{core_package_dir} to file_test support so that you can add --exclude-dump-file-prefix=%{core_package_dir} to ARGS? (it seems fine if you want to leave that as a TODO, but the test file would then need to be removed from the PR)

Looking into that. I'm a bit of a testing pedant, so would prefer not to commit the change untested, and happy to frontload the test infrastructure improvements necessary to ensure coverage.

Ended up that I needed to add generic functionality to file_test_base (since it errors on unknown characters after %) - so allowed derived classes to expose a collection of replacements, and used those...

Copy link
Contributor

@jonmeow jonmeow left a comment

Choose a reason for hiding this comment

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

A couple more things to change:

  1. testing/file_test/README.md is trying to document syntax, can you add a note about this under where it says "Supported replacements within arguments"?

  2. In testing/file_test/testdata, can you add a test similar to args.carbon, with something like ARGS: abc %{replacement} %s?

    • In testing/file_test/file_test_base_test.cpp, if you add the new test file where args.carbon is (the early return), you can validate that the replacement is correctly round tripping.
    • Note this directory also has an autoupdate script (a more trivial one than toolchain's)

testing/file_test/file_test_base.h Outdated Show resolved Hide resolved
@jonmeow
Copy link
Contributor

jonmeow commented Aug 22, 2024

  1. In testing/file_test/testdata, can you add a test similar to args.carbon, with something like ARGS: abc %{replacement} %s?

Or really, even just add %{replacement} to args.carbon. It doesn't need to be a separate file, it can just be added to the existing test.

@dwblaikie
Copy link
Contributor Author

A couple more things to change:

  1. testing/file_test/README.md is trying to document syntax, can you add a note about this under where it says "Supported replacements within arguments"?

  2. In testing/file_test/testdata, can you add a test similar to args.carbon, with something like ARGS: abc %{replacement} %s?

    • In testing/file_test/file_test_base_test.cpp, if you add the new test file where args.carbon is (the early return), you can validate that the replacement is correctly round tripping.
    • Note this directory also has an autoupdate script (a more trivial one than toolchain's)

Ah, for sure - thanks for the pointers. Done!

@jonmeow
Copy link
Contributor

jonmeow commented Aug 22, 2024

Just a reminder for the README.md edit, in case it was missed. :)

@dwblaikie
Copy link
Contributor Author

Just a reminder for the README.md edit, in case it was missed. :)

Ah, thought I'd done that - sorry I missed it. Added some now.

Copy link
Contributor

@jonmeow jonmeow left a comment

Choose a reason for hiding this comment

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

Thanks! All looks good here.

@jonmeow jonmeow enabled auto-merge August 22, 2024 20:20
@jonmeow jonmeow added this pull request to the merge queue Aug 22, 2024
Merged via the queue into carbon-language:trunk with commit 1e1034e Aug 22, 2024
8 checks passed
@dwblaikie dwblaikie deleted the debug_info_on_by_default branch August 22, 2024 20:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants