Skip to content

fix(parse): make template-argument-list optional #473

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

Merged
merged 3 commits into from
Jun 17, 2023

Conversation

JohelEGP
Copy link
Contributor

@JohelEGP JohelEGP commented May 30, 2023

Resolves #469.

Testing summary.
100% tests passed, 0 tests failed out of 508

Total Test time (real) = 120.76 sec
Acknowledgements.

@JohelEGP JohelEGP force-pushed the template-argument-list branch from 4f721a0 to 07eef02 Compare May 30, 2023 02:29
@hsutter
Copy link
Owner

hsutter commented May 30, 2023

Looks pretty good, thanks! Did you also check the other places template_arg gets used to ensure that the code will handle an empty list correctly?

For example, unqualified_id_node::visit should probably still emit the template_arms_tag even though no arguments will follow, so that the visit doesn't lose the empty list.

Also, does lowering emit the empty list? I don't see a .cpp result for the regression test, and I wouldn't expect it seeing cppfront.cpp line 1669. These probably should take the cue that a non-default .open_angle means there's a list, and allow an empty list?

@JohelEGP
Copy link
Contributor Author

Did you also check the other places template_arg gets used to ensure that the code will handle an empty list correctly?

No.
Testing didn't catch anything.
I'll have to investigate that.

Also, does lowering emit the empty list?

Seems like it doesn't.
The added test happens to work without it.

For example, unqualified_id_node::visit should probably still emit the template_arms_tag even though no arguments will follow, so that the visit doesn't lose the empty list.

I don't see a .cpp result for the regression test, and I wouldn't expect it seeing cppfront.cpp line 1669. These probably should take the cue that a non-default .open_angle means there's a list, and allow an empty list?

I'll have to investigate those places.

I don't see a .cpp result for the regression test,

I have to investigate why https://github.com/modern-cmake/cppfront
isn't picking up the tests I add
(there's always 250 tests on main and my branches).

I do test it manually in my developer environment.
I suppose I could just copy the generated Cpp1 source back to regression-tests.

@hsutter
Copy link
Owner

hsutter commented May 30, 2023

I suppose I could just copy the generated Cpp1 source back to regression-tests.

Yes, to regression-tests/test-results please. The generated Cpp1 should be checked in with each new test. That should happen automatically if you re-run all of the regression tests to make sure there are no diffs, using the test-results/run-tests.bat or equivalent .sh.

@JohelEGP
Copy link
Contributor Author

That should happen automatically if you re-run all of the regression tests to make sure there are no diffs, using the test-results/run-tests.bat or equivalent .sh.

Unfortunately, I'm on Linux.
https://github.com/modern-cmake/cppfront does it platform-agnostic.
As I suspected, it only picks up tests that have an .output in test-results.
I'll see about enhancing it to generate all those, as it's convenient for developers.

@JohelEGP
Copy link
Contributor Author

Testing didn't catch anything.

Of course, silly me. That's why I made this PR.

The added test happens to work without it.

Looks like I didn't actually run it.

I checked P2601 To make redundant empty angle brackets optional for class template argument lists
for a context that requires <> and enhanced the test with it.

@JohelEGP JohelEGP force-pushed the template-argument-list branch from 07eef02 to a5227e0 Compare May 30, 2023 22:49
@JohelEGP
Copy link
Contributor Author

JohelEGP commented Jun 3, 2023

These probably should take the cue that a non-default .open_angle means there's a list, and allow an empty list?

Applied.

@JohelEGP
Copy link
Contributor Author

JohelEGP commented Jun 7, 2023

100% tests passed, 0 tests failed out of 508

FYI, these are the tests under regression-tests.
Currently, each one split into codegen and build tests.
codegen (invokes cppfront),
codegen/check (checks the outputs with that under test-results), and
codegen/update (copies the outputs to under test-resutls in developer mode).
Excluding the *-error tests,
build (invokes the Cpp1 compiler),
build/execute (executes the compiled Cpp2 program), and
build/update (copies the outputs to under test-resutls/compiler-major_version in developer mode).

It has become evident that I'm missing build/check analogous to codegen/check.

@hsutter
Copy link
Owner

hsutter commented Jun 17, 2023

Looks good, thanks!

@hsutter hsutter merged commit 85a1e8a into hsutter:main Jun 17, 2023
@JohelEGP JohelEGP deleted the template-argument-list branch June 17, 2023 22:19
zaucy pushed a commit to zaucy/cppfront that referenced this pull request Dec 5, 2023
* fix(parse): make template-argument-list optional

* refactor: use `open_angle` as cue for temp arg list

* test: add `test-results/gcc-13`
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.

[BUG] Empty template argument list is ill-formed
2 participants