User Details
- User Since
- Apr 6 2017, 1:42 AM (311 w, 3 d)
- Roles
- Administrator
Tue, Mar 21
I landed the fix to unbreak our crashes and explored a bit of the alternative solution.
- Add a test for block pointers
Mon, Mar 20
- rename test file to GH61441.cpp, add a comment this checks for no crash
- Update outdated comment
Fri, Mar 17
I suspect D145641 won't land today in the EU working hours.
My suggestion would be to revert this patch and reland together with D145641.
This should be in line with @ChuanqiXu's suggestions to wait for a response today.
Thu, Mar 16
@bgraur, FYI, hope waiting for a day works for this case.
Reverting the RVO breaks some coroutine code we have. The short repro is https://gcc.godbolt.org/z/1543qc8Ee (code at the end of the post, very similar to the deleted constructor in warn-throw-out-noexcept-coro.cpp):
The RVO seems to be mandated by the standard and D145641 is in the works to fix this.
Thu, Mar 9
I don't have a lot of experience in codegen, so will let Aaron and Richard do the review.
Feb 7 2023
Feb 3 2023
Feb 2 2023
Feb 1 2023
I don't see any issues with this, so happy to LGTM. @rsmith any concerns on your side?
@usaxena95 could you give an example of the code that fails the assertion? Is it some of the tests?
Jan 31 2023
Jan 25 2023
Jan 24 2023
Ah, I keep forgetting that the unknown attributes are fine. My experiments with MSVC suggest that it will increase the struct size: https://gcc.godbolt.org/z/offYrhn4K.
libc++ uses msvc::no_unique_address, which seems to work, but is MSVC-specific.
For the record, the corresponding standard change is P1008R1.
To describe the problem, the following code breaks in C++20:
struct WantAggregate { WantAggregate(const WantAggregate&) = delete; int a; int b; };
Jan 23 2023
Jan 20 2023
Randomly chiming in here.
I never had a good model of where CorrectDelayedTyposInExpr, but wanted to note that ActOnFullExpr also calls it. This may be fine, I just wanted to mention it as it stood out.
Jan 17 2023
Jan 16 2023
LGTM
Jan 13 2023
Slight NIT: we might want to initialize End and iterator_adaptor_base::I with WrappedIteratorT End = WrappedIteratorT() to avoid uninitialized data members when WrappedIteratorT is a pointer.
This does not happen in practice right now, but if someone actually starts using this constructor in the future, we will at least not have UB.
Sorry, didn't see your comment when submitting mine.
The change seems reasonable. Could you add a test with a crasher repro and some validation of the generated IR?
LGTM! Looks much cleaner now, BTW.
LGTM, would be happy to get those emails.
I have not checked, but I would not be surprised if we hit the stack size limits with asan enabled
@usaxena95, maybe reduce the number of instantiations from 10001 to 1001 or 101? It should not change the intention of the test and fix this failure.
There is potentially a way to move less code by keeping all declarations at place and only moving bodies of definitions of constructors and destructors to the .cpp file.
Not sure what's preferable (less code moves vs more functions inline in the header), @kadircet do you have an opinion?
Jan 12 2023
LGTM, mostly based on the previous review.
I have not noticed any significant changes, so have not looked too cautiously. Let me know if there are any particular places that you someone to take a look at before landing.
Jan 11 2023
LGTM, but please address the NITs before submitting.
LGTM, thanks for fixing this!
Jan 10 2023
Are there any uses of u8 string literals with formatted_raw_ostream outside tests? What was the effect on the final behavior here?
LGTM. The presubmit failures are definitely unrelated as they are in a lit test for a different tool.
LGTM to unbreak the buildbots, this change definitely does not change semantics.
Jan 9 2023
I think the only major problem is not checking for error case when accessing TransReq, the rest are NITs.
Thanks for the change! Will be happy to LGTM it as soon as the access to TransReq is fixed.
Jan 3 2023
Should access checks should happen in the context where concept is written or where it's used? Is there a standard wording around it?
If access-checking should happen where concept is defined, having a hard error seems fine because of the wording you quoted:
If the substitution of template arguments into a requirement would always result in a substitution failure, the program is ill-formed; no diagnostic required.
The program is ill-formed and we show the diagnostic even though it's not required.
Dec 23 2022
LGTM to unbreak clangd, this seems to pop up a lot for Chrome developers.
Dec 22 2022
Dec 21 2022
- Use kubernetes resource YAML for deployment instead of terraform config. I had problems updating our terraform deployment, it seems broken. I will follow up by moving existing clangd buildbot out of terrafrom too, the kubectl-based workflow seems prefereable.
- Set version of the new docker image to 0 (script updates it on build).
LGTM
Dec 20 2022
- fix a typo in /vol/ccache
- use password from clangd worker (approved by Galina in our email exchange)
- leave out the targets and checks, use defaults that build and test everything
- Use $CC and $LD instead of the wrong compilers in run.sh
Dec 19 2022
Sending this out early to have it reviewed by the time I get the required credentials.
This will also require the corresponding configuration changes in our GCP changes before being submitted.
Dec 15 2022
Dec 14 2022
LGTM! Thanks a lot for driving this to conclusion! Please note that there is one small NIT you may want to fix before landing this.
UB definitely explains why we saw the bugs. It's good that we caught it before this landed.
Dec 12 2022
Since the errors only shows up in modular builds, I would look closely for bugs inside ASTReader/ASTWriter.
Also, it seems that ArrayFiller is not taken in to account in computeDependence and maybe it should be. I am not 100% sure, though: if ArrayFiller is only used for non-dependent code, it should not case this bug. It also does not explain the variation between modular and non-modular builds.
This LG, but not accepting as I believe libc++ is still broken if built with modules, right?
I have run check-cxx locally and it seems to work, but I suspect that's not using modules by default. I have clicked through the links in phrabricator and the errors seem to come from the latest uploaded diff.
Let me know if I'm missing something.
Dec 9 2022
Dec 7 2022
LGTM. BTW, feel free to send small changes like these without review. The common jargon for them in LLVM is NFC (non-functional change)
I have sent a follow-up NFC change to fix a typo in the documentation of --ranking-model=heuristics. Note that I decided to avoid changing the order of options for --ranking-model in the documentation, this didn't seem important.
- Add a comment to CMakelists.txt explaining motivation for the option
- Add #include "Feature.h"
- Revert changes to the order of compiler flags to keep this change more focused
- clang-format
- replace #ifndef with #if ! in ClangdMain.cpp
Thanks, LGTM!
Dec 5 2022
I have two major comments and also inline NITs. Not sure if we should block on those, just wanted to hear your opinions:
- InitListExpr and CXXParenInitListExpr have some code in common. Code duplication is substantial and I think sharing the common implementation is warranted. E.g. if some code would want to change something with ArrayFiller or make a use of it, the work will probably need to be duplicated. To reiterate what I mentioned earlier, I believe deriving CXXParenInitListExpr from InitListExpr is not a very good option, but we might explore other possibilities: a base class or factoring out common code in a separate struct. I believe this could be done with a follow-up change, though, as the amount of changes required does not seem too big, I would be happy with first landing this patch and then cleaning up with a follow-up change.
- Did we explore the possibility of modelling this differently and somehow avoiding making CXXParenInitListExpr an expression. This seems to borrow from InitListExpr, but maybe for the wrong reasons. Braced initializers can actually occur in expressions positions, e.g. int a = {123}. However, CXXParenInistListExpr cannot occur in expression positions outside initializations, e.g. having CXXFunctionalCastExpr for aggregate_struct(123) feels somewhat wrong to me and seems to lead to confusing error messages too. Maybe we should model it as a new expression that contains both the type and the argument list? I.e. more similar to CXXConstructExpr?
- Merge DecisionForest.cpp and *_disabled.cpp, change #ifndef to #if
- Add decision_forest to the feature string
- Set Heuristics as the default code completion model in tests
- Do not compile decision forest tests when it's disabled
- Remove the use of preprocessor for defining the default value of -ranking-model flag to clangd
Dec 2 2022
Dec 1 2022
This seems to build in both modes, but I still need to figure out what to do with tests with decision forest off.
Also, maybe there is a way to minimize the use of preprocessor even more.
Nov 28 2022
LGTM with a few NITs. Thanks!
Thanks for the explanation. LGTM! And thanks for adding an assert.
Thanks for addressing the comments and sorry for a wait before the comments.
Please see the comment about syntactic form, I might be holding it wrong, but it looks like we actually loose the syntactic form completely in this case.