How to tackle P1787 "Declarations and where to find them"?

Clang Frontend
1

Hello. I’m working on tests for CWG issues resolved in P1787, and I’m not sure what’s the best way to submit patches due to sheer amount of issues it resolves (70+). One patch per issue? One patch per group of issues, e.g. “Lookup completeness”? Single patch for the whole paper?

2 Likes
2

Thanks for working on that!
It’s also not clear to me how to tackle this paper in case it changes things not tracked by a specific issue.

To your question, if it’s just test code, i think multiple issues per patch is fine, but if there need to be change in the compiler, then 1 patch per change would be preferable

3

Thank you. I’m not getting into fixing those issues in compiler, at least just yet. I tried it once with CWG1960, but got overwhelmed by complexity of name lookup code.

4

There is no reasonable way to approach this paper. We ran into the same thing with one of the C DRs that I affectionately labelled “39 unrelated questions about C89”. :slight_smile: The changes resolve core language issues:
36 110 138 191 255 271 279 338 360 386
399 405 418 536 554 562 563 600 607 852
952 1028 1200 1252 1291 1478 1500 1616 1729 1771
1818 1820 1821 1822 1828 1829 1835 1837 1839 1841
1884 1894 1896 1898 1900 1907 1908 1936 2007 2009
2058 2062 2065 2070 2165 2199 2213 2331 2370 2396
2413

In addition, issues

325 361 1089 1635

are partially resolved.

and we test DRs by adding test cases and special comment markup to files in llvm-project/clang/test/CXX/drs at main · llvm/llvm-project · GitHub. My recommendation is to add tests to the appropriate files within this directory. If the individual tests are small, I think it’s fine to group them into a single patch for review. However, if any of the tests are larger or more complicated (basically, something expected to require some discussion), then I’d split those out into their own patch so we can still land the easy stuff quickly.

1 Like
5

Would it be too much pressure on reviewers if I submit one patch per issue by default? I got through two dozens of them already, and I believe the only trivial ones are pure wording changes that doesn’t affect implementation.

Maybe I treat those issues more seriously than they deserve, but it feels that once an entry on DR status page becomes green, barely anybody seriously look at test code.

6

If the reviews all come in at the same time, that’s probably going to add a fair amount of review pressure for folks (there’s > 60 issues in total and review throughput usually slows down this time of year due to the holidays), but if they’re spaced out a bit, I don’t think it’s a problem. So I’d say start with one issue per patch and be flexible if the reviewers ask for something different for subsequent patches.

1 Like