Page MenuHomePhabricator

Fix for: Bug 5941 - improve diagnostic for * vs & confusion
ClosedPublic

Authored by ryee88 on Feb 6 2016, 1:25 PM.

Event Timeline

ryee88 updated this revision to Diff 47097.Feb 6 2016, 1:25 PM
ryee88 retitled this revision from to Fix for: Bug 5941 - improve diagnostic for * vs & confusion.
ryee88 updated this object.
ryee88 added a subscriber: cfe-commits.

Ping-- can someone look at this review?

Needs test coverage

ryee88 updated this revision to Diff 47924.Feb 13 2016, 8:29 PM

Added unit test as per review comments.

ryee88 updated this revision to Diff 47925.Feb 13 2016, 8:32 PM

Sorry-- re-uploading the diff. Didn't notice I dropped the code changes.

Since this is just a wording change, presumably the diagnostic is already
tested in an existing file - perhaps you could exetendi that test to cover
this functionality (we try not to add new test files too much - better to
test functionality once/in one place as much as possible (both for ease of
reading/updating/understanding the test suite, and for speed of execution
(process startup time makes up a significant portion of the test execution
time, so reducing the total number of test files/commands is useful there)))

ryee88 updated this revision to Diff 48149.Feb 16 2016, 10:01 PM

Keeping the number of test files to a minimum makes sense.

Couldn't find an existing test for this diagnostic. I did find a cxx-reference.cpp that tests reference diagnostics so this seems like a reasonable location for this new test.

any comments?

ryee88 updated this revision to Diff 49280.Feb 26 2016, 6:57 PM

Moved test to a file that checks for similar diagnostics.

Are there not any existing cases that test this diagnostic?

Could you just update some existing cases to cover this? (it looks like, in the same file, the test case added for PR 6117 could be expanded to test this as well, perhaps?) It looks like that's the original test for this diagnostic, so it'd be a good/right place to test the extra text and add any extra variants that have become relevant.

ryee88 added a comment.Mar 1 2016, 6:52 PM

Yeah you’re right about this one being the original variant. I missed it when I was reading through the tests in this file.

I’ve reworked it to reflect the new variants.

ryee88 updated this revision to Diff 49577.Mar 1 2016, 6:55 PM
dblaikie accepted this revision.Mar 1 2016, 11:31 PM
dblaikie added a reviewer: dblaikie.

Looks great - thanks!

This revision is now accepted and ready to land.Mar 1 2016, 11:31 PM
ryee88 added a comment.Mar 3 2016, 5:46 PM

Thanks! I don’t have commit permissions; can you submit for me?

This revision was automatically updated to reflect the committed changes.