User Details
- User Since
- Sep 28 2015, 1:40 PM (383 w, 2 d)
Mon, Jan 16
Jun 3 2020
Sorry for breaking the build and thanks for the fixes!
Implemented njames93's comments
Jun 2 2020
May 25 2020
Fix comments
Aaron, thank you very much for taking the time to review this PR.
May 21 2020
Thanks for doing this! I didn't work on the YAML parser before, so I cannot give formal approval.
May 14 2020
Ping :-)
I'm looking for directions on what the next steps are.
May 4 2020
Could be - did it handle it correctly for your include case here?
Apr 30 2020
This change breaks modernize-use-using fixits.
The code
typedef int A, B;
is replaced into
using A = int; using B = int;
when directly applying fixits with clang-tidy. But it is replaced into
using A = int;
Apr 27 2020
Apr 17 2020
Apr 16 2020
Hi @vsk, sorry, I pushed that branch by accident to the wrong remote.
Committed as 6d2f73f821ed5ea584869924b150ac2e6e65c12e
Fix typo
PR is here: https://reviews.llvm.org/D78290
I'll add it to Compiler.h
Apr 15 2020
Implement review comments (Thanks!)
Apr 14 2020
Thanks for the comments so far.
I'm a bit lost now. Which changes that cannot wait for the next PR do you see necessary to get this check merged?
Apr 12 2020
Apr 10 2020
Thanks @njames93, this is a good examples which I did not consider yet. I agree that only transforming
the second loop would make the code worse.
Apr 9 2020
Apr 8 2020
Apr 7 2020
Review comments
Review comments
Apr 6 2020
Jan 9 2020
Jan 8 2020
Nov 10 2019
Did you consider to warn on copy constructors/assignment operators take a their arguments by non-const reference, and suggest the user to turn them into const references? This seems like a more useful (and easier) check to me.
The link above contains Ideally, the copy operator should have an idiomatic signature. For copy constructors, that is T(const T&); and for copy assignment operators, that is T& operator=(const T&);..
Have you run your check over the LLVM/clang source base and seen true positives/false positives?
Nov 6 2019
Nov 2 2019
Thanks, fixed!
Oct 30 2019
Friendly Ping
Oct 20 2019
- Update documentation
Oct 19 2019
Exactly due to the issue you are fixing here, we ended up disabling the complete check because we didn't want to live with the warnings it produced on -Wextra.
Therefore, I'm actually strongly in favor to enable the option by default.
Oct 9 2019
Sep 29 2019
I believe in this case, we can get away with a simpler analysis. Because this is an prvalue,
there are less operations that can be done to it, and it seems easy to just list those here. Also, we want
to conservatively disallow "const" access to non-public member variables/functions, and I don't see
how this would elegantly integrate with using the ExprMutationAnalyzer.
- Implement review comments
Sep 26 2019
Sep 6 2019
Sep 5 2019
The discussions on the bug did not provide further insight (until now).
Aug 27 2019
Nice! Having no false-positives is most important because this is enabled by default.
Aug 26 2019
I'm seeing the same issue Not all CodeGen sections are inside any Frontend section! with python 3.7.1. Json:
Aug 24 2019
Aug 23 2019
Yes, it means that the automatic inference of Pointer/Owner from base classes has the same issues as all of those automatic inferences: Once can construct a case where they are not true.
So for having no false-positives at all, we should avoid this inference by default and not consider MutableArrayRef to be a gsl::Pointer.
Instead, we disable the analysis when it sees an unannotated class (like here, MutableArrayRef).
When I understand you correctly, you are thinking about
the cases
OwningArrayRef getRef();
Generally, ArrayRef is also a candidate. But there we have the complication that OwningArrayRef indirectly inherits from ArrayRef. One is a gsl::Owner, the other a gsl::Pointer, and we
need to make sure that we handle it correctly when OwningArrayRef is accessed through its base class which is then seen as a Pointer.
I would thus defer this part to a later PR.
Also it feels a bit weird to change the ownership semantics in a derived class, I bet that would violate the Liskov substitution principle.
And then we see that llvm::OwningArrayRef inherits from llvm::ArrayRef ... But maybe this direction (strengthening a Pointer into an Owner)
is okay. We'll need to go though the call rules, and see if something breaks when the caller passes an Owner to a function that takes a base class (i.e. Pointer).
My initial reaction is that this should work in general. An Owner is like a Pointer that points to {static}.
Aug 22 2019
Yes, for libc++ we could add the annotations to its source code, and eventually this would be the cleaner solution.
Currently, that is neither sufficient (because one could use libstdc++, MSVC or an older libc++ version) nor necessary (the inference we need for libstdc++ / MSVC also works for libc++),
so it's currently low on our priority list.
Aug 21 2019
In the false-positive example, after the DerivedToBase, we see a constructor call which I think is the copy constructor.
- We should consider MutableArrayRef to be a gsl::Pointer according to the paper, because it publicly derives from one.
- Also in the paper, the copy constructor does should copies the pset of the argument instead of making the pointer point at the argument.
What do you think?
Aug 20 2019
This change might be the cause for the false-positive in https://github.com/mgehre/llvm-project/issues/45. Could you check?
I now add the attributes to all redeclarations to make the logic a bit easier to follow.
- Put OwnerAttr/PointerAttr on all redeclarations
- clang/lib/Sema/SemaAttr.cpp: Use Attribute::CreateImplicit as requested in review
Aug 19 2019
I struggle myself to fully understand the motivation behind the bug report. This is certainly not code that I would write.
So I don't really care about that kind of code and I didn't want to be in the way of other people's way of writing C++.
Aug 16 2019
I now start to wonder if we should instead put the attribute on all declarations (instead of just the canonical). Later redeclarations automatically inherit the attributes.
This would make both the checking easier (just check any declaration, no need to obtain the canonical first), and remove the special
case for adding to the definition of templated record for ClassTemplateDecls.