- User Since
- Apr 6 2017, 1:42 AM (124 w, 2 d)
Thu, Aug 22
LGTM. Kill it with fire
Wed, Aug 21
This actually seems useful. If one starts the search at operator bool, it's nice to see its implicit calls too.
What are the cons of having this?
Tue, Aug 20
- Change / to
- Expose the existing helper instead of introducing a new one
Mon, Aug 19
LGTM from my side (and a few NIT, but up to you whether to apply them)
Fri, Aug 16
Wed, Aug 14
Thanks for providing examples of error messages, also agree they look fine.
If we find bad error messages later, we could revisit the presentation strategy.
- Rename the helper function
- Remove mention of 'out-of-line definition' from a comment
Surfacing errors to the users in those cases is definitely something we need to do, thanks!
How do they look in practice? In particular, should we add more context information (either in clangd or in the VSCode extension) to those errors, now that we know they are shown to the users?
Something like Failed to apply a fix: input range is invalid vs input range is invalid.
This patch mentions there is code currently that "reconstructs the mapping". Why not remove it in this patch? Where is it used if we actually serialize the mapping?
Do we have any measurements? How faster is this in particular use-cases? How bigger
Tue, Aug 13
I did some experiments with adding something similar to the ErrorExpr Aaron suggest. It has this flag set and does not get removed from the AST.
I believe the best way to address Aaron's comments is to add tests mentioning this expression instead of trying to catch TypoExpr, which normally get removed from the AST.
Alternatively, we could keep an equivalent of set<Expr*> InvalidExprs in ASTContext. Gives the same computational complexity, but has a higher constant overhead factor.
Does that look reasonable?
Mon, Aug 12
The heuristics for properly rendering the comments are probably a good way to go.
I'd say this change is still a step in the right direction - text blocks in formatted strings should be properly escaped to avoid being interpreted like markdown constructs.
Any structure we want to have should be explicit.
Are expression bits scarce?
We don't any checks if expressions contain errors now, we simply drop too many invalid expressions and never put them into the AST.
It's impossible to do the measurements at this point, but it would be nice if adding those checks was cheap.
Exactly right, thanks for summing this up!
LGTM. Great example for the test case! It will definitely stay valid even if we fix all problems caused by RecursiveASTVisitor!
Gentle ping. @rsmith, please take a look when you have time.
I actually think it's good to have it as an explicit option. Qualified and unqualified lookup are so vastly different use-cases, that having the flag that discriminates them as a function parameter looks like a better trade-off.
Besides, RAII objects are more code and more complicated.
We should also merge this into the release branch if it's not too late yet.
Wed, Aug 7
LGTM, but please print the function template arguments uniformly before landing this (to avoid different forms of outputs inside the same test).
Tue, Aug 6
One important comment about somehow distinguishing multiple decls with the same name.
- Add the missing newline
- Make sure the test actually runs IncludeFixer.cpp
Mon, Aug 5
This is just a proposal, there are probably other ways to reach better readability, e.g. group some of those parameters into a struct.
But let me know what you think, happy to refactor in a slightly different manner or simply drop this revision.
- Remove accidental change from revision
LGTM from my side, a few optional NITs.
Feel free to land as soon as @hokein stamps.
The fix looks good.
Fri, Aug 2
@hans, could we merge this commit into the release branch?
Could this be handled outside clangd? If not, what are the complications?
Thu, Aug 1
Neat, thanks for the cleanup! A few suggestions from me, no major comments.
SG, just wanted to make sure it's on your list ;-)
- Update a comment
Wed, Jul 31
LGTM from my side
- Add a test
An experiment with popping on expression evaluation context failed, I couldn't find a good way to fix the problems described above.
Typo correction can and will run after the evaluation context where expression created is popped and the diagnostic we produce depends on the results of typo correction.
Emitting diagnostics on some, but not all context pops could be an option, but classifying those seems to be hard, would require looking closely at every expr eval context push.