| rsmith | |
| martong | |
| shafik | |
| aaron.ballman |
Changes:
| clang/lib/AST/ASTImporter.cpp | ||
|---|---|---|
| 8764 | Yes, that is indeed not so good. The point is that, we should execute the APValue tests as well when we invoke ninja check-clang-astmerge. (Some people do not execute the whole check-clang verification when they change only the ASTImporter, this way the edit-test cycle can be faster) Can we have a symbolic link from test/PCH/APValue.cpp to test/ASTMerge/APValue.cpp? Probably that would work on *nix, but not on Windows (https://stackoverflow.com/questions/5917249/git-symlinks-in-windows). | |
Sorry for the long wait.
Changes:
Thanks for addressing the issues. The ASTImporter related changes look good to me now.
| clang/include/clang/AST/APValue.h | ||
|---|---|---|
| 517 | reserveVector per naming conventions | |
| clang/include/clang/AST/ASTContext.h | ||
| 281 | Why are you getting rid of this? It seems like we would still want these cleaned up. | |
| clang/include/clang/AST/Expr.h | ||
| 962 | Enumerator -> Enumerators | |
| clang/include/clang/AST/TextNodeDumper.h | ||
| 149 ↗ | (On Diff #217600) | Good catch -- this pointed out a bug in the class that I've fixed in r372323, so you'll need to rebase. |
| clang/lib/AST/APValue.cpp | ||
| 177 | Why is this no longer a pointer to const? | |
| 764 | Can this function be marked const? | |
| clang/lib/AST/ASTImporter.cpp | ||
| 8764 | Conflict markers? | |
| 9017 | Please don't use auto as the type is not spelled out in the initialization. | |
| 9022 | Please don't use auto as the type is not spelled out in the initialization. (Same elsewhere, I'll stop commenting on them.) | |
| clang/lib/AST/TextNodeDumper.cpp | ||
| 691–692 | You'll have to rebase this too -- these changes shouldn't be needed any longer. | |
| clang/lib/Sema/SemaDecl.cpp | ||
| 11725 | Can elide braces. | |
| clang/lib/Serialization/ASTReader.cpp | ||
| 9598 | auto * | |
| 9604–9605 | auto * | |
| 9611 | auto * | |
| 9622–9628 | These names don't match the current coding conventions. | |
| 9631 | You should add parens here to avoid diagnostics. | |
| clang/lib/Serialization/ASTWriter.cpp | ||
| 5492 | Name doesn't match coding conventions. | |
| 5503 | const auto * | |
| 5525 | const auto * | |
| 5529 | const auto * | |
| 5544 | Unrelated change? | |
| clang/lib/Serialization/ASTWriterStmt.cpp | ||
| 42 | It seems like there's a lot of unrelated formatting changes in this file that should not be part of the patch. | |
Fixed most changes requested by @aaron.ballman
test are currently no valid anymore see comments for why.
| clang/include/clang/AST/ASTContext.h | ||
|---|---|---|
| 281 | when i added APValueCleanups i wasn't aware that there were a generic system to handle this. but with this patch APValue a cleaned up using the generic ASTContext::addDestruction. | |
| clang/include/clang/AST/TextNodeDumper.h | ||
| 149 ↗ | (On Diff #217600) | i took a look at the revision. there is a big difference is the quality of output between APValue::dump and APValue::printPretty. i think it is possible to come quite close to printPretty's output even without the ASTContext. this would require having a default PrintingPolicy and improving dump in this patch i was relying on the -ast-dump output for testing. i would need to find an other testing strategy or make the improvement to APValue::dump first. |
| clang/lib/AST/APValue.cpp | ||
| 177 | when imporing or deserializing, we reserve the space for elements and then import/deserialize element directly in place. so the buffer storing them is not const. that said i saw that the normal construction cast away the const. | |
| 764 | this function gives access to non-const internal data. this function is private so the impact is quite limited. | |
| clang/lib/AST/APValue.cpp | ||
|---|---|---|
| 177 | never mind this change wasn't needed. | |
| clang/include/clang/AST/APValue.h | ||
|---|---|---|
| 517 | This was marked as done but is still an issue. | |
| 542–545 | Rather than add this private bit in the middle of the public interface, you can move this to the existing private parts. | |
| 616 | We're horribly inconsistent in this class, but because the other private member functions go with it, this should probably be GetMemberPointerPathPtr(). Maybe rename the get/setLValue methods from above as well? | |
| clang/include/clang/AST/ASTContext.h | ||
| 281 | I don't see any new calls to addDestruction() though. Have I missed something? | |
| clang/include/clang/AST/TextNodeDumper.h | ||
| 149 ↗ | (On Diff #217600) |
Yes, there is.
That would be much-appreciated! When I looked at it, it seemed like it may not be plausible because Stmt does not track which ASTContext it is associated with the same way that Decl does, and changing that seemed likely to cause a huge amount of interface churn.
The issue resolved by r372323 was that we would crash on certain kinds of AST dumps. Specifically, the default AST dumper is often used during a debugging session to dump AST node information within the debugger. It was trivial to get that to crash before r372323, but with that revision, we no longer crash but get slightly uglier output (which is acceptable because it's still human-readable output). I'm sorry for causing extra pain for you here, but I didn't want the fix from this review to accidentally become an enshrined part of the API because it's very easy to forget about this use case when working on AST dumping functionality. |
| clang/lib/AST/APValue.cpp | ||
| 764 | That makes it harder to call this helper from a constant context. I think there should be overloads (one const, one not) to handle this. | |
| clang/test/ASTMerge/APValue/APValue.cpp | ||
| 2 | Can remove the spurious newline. Also, it seems this file was added with svn properties, was that intentional (we don't usually do that, FWIW)? | |
| 3–4 | no need for -x c++ is there? This is already a C++ compilation unit. | |
| 29 | Are you planning to address this in this patch? Also, I think it's FixedPoint and not FixePoint. | |
| clang/test/PCH/APValue.cpp | ||
| 1 ↗ | (On Diff #221169) | Spurious newline. |
| 2 ↗ | (On Diff #221169) | -x c++ ? It's really unfortunate that these test files are identical copies in different directories. |
| 28 ↗ | (On Diff #221169) | Same comment here. |
fixed some changes.
see comments for others.
| clang/include/clang/AST/APValue.h | ||
|---|---|---|
| 517 | sorry | |
| 616 |
this class has many flaws. but is far too broadly used to fix. | |
| clang/include/clang/AST/ASTContext.h | ||
| 281 | the modification to use addDestruction() was made in a previous revision (https://reviews.llvm.org/D63376). | |
| clang/include/clang/AST/TextNodeDumper.h | ||
| 149 ↗ | (On Diff #217600) | no worries, i wrote the original bug. i added APValue::dumpPretty which has almost the same output as APValue::printPretty but doesn't need an ASTContext. and is used for TextNodeDumper. |
| clang/lib/AST/APValue.cpp | ||
| 764 | this helper is not intended to be used outside of importing and serialization. it is logically part of initialization. | |
| clang/test/ASTMerge/APValue/APValue.cpp | ||
| 2 | it wasn't intentional, i added via git add i don't think i did anything weird. is it a problem ? | |
| 3–4 | i don't know if it is normal. but i am getting an error hen i am not using -x c++ | |
| 29 | i don't intend to add them in this patch or subsequent patches. i don't know how to use the features that have these representations, i don't even know if they can be stored stored in that AST. so this is untested code. | |
| clang/test/PCH/APValue.cpp | ||
| 2 ↗ | (On Diff #221169) | they are almost the same. the second run line is slightly different. i had a version with both run line in the same file. but martong was worried that people running partial test would only not get all test relevant for what they were testing. after through. ASTMerge requires both serialization and importing so it would probably be sufficient ? |
| clang/include/clang/AST/APValue.h | ||
|---|---|---|
| 616 | Agreed -- I wasn't suggesting to fix the whole class, but just the new APIs that we add to the class. It looks like the private functions most consistently use a capital letter in this class, unfortunately. Best to stick with the local convention when in conflict. | |
| clang/include/clang/AST/ASTContext.h | ||
| 281 | Ahhh, thank you for the explanation, I was missing that context. | |
| clang/include/clang/AST/PrettyPrinter.h | ||
| 203 | Wether -> Whether | |
| clang/lib/AST/APValue.cpp | ||
| 537 | Since you're touching the code anyway, this can be const auto *. | |
| 599 | Are you sure this doesn't change behavior? See the implementation of ASTContext::getAsArrayType(). Same question applies below. | |
| 614 | const auto * and same question about behavior changes. | |
| 764 | Nothing about this API suggests that. The name looks like a generic getter. Perhaps a more descriptive name and some comments would help? | |
| clang/test/ASTMerge/APValue/APValue.cpp | ||
| 2 | No idea; I'm on a platform where file modes are ignored. You should probably drop the svn property. | |
| 3–4 | There's no %s on that line for the source file, which is why you get that diagnostic. I'm not certain what that RUN line does, actually -- it does an AST merge with... nothing... and then prints it out? If that's intended, then you only need the -x c++ on that one RUN line. | |
| 29 | I don't think llvm_unreachable makes a whole lot of sense unless the code is truly unreachable because there's no way to get an AST with that representation. By code inspection, the code looks reasonable but it does make me a bit uncomfortable to adopt it without tests. I suppose the FIXME is a reasonable compromise in this case, but if you have some spare cycles to investigate ways to get those representations, it would be appreciated. | |
fixed most comments
| clang/lib/AST/APValue.cpp | ||
|---|---|---|
| 599 | i ran the test suite after the change it there wasn't any test failures. but the test on dumping APValue are probably not as thorough as we would like them to be. i think the best way the have higher confidence is to ask rsmith what he thinks. | |
| clang/test/ASTMerge/APValue/APValue.cpp | ||
| 29 | the reason i proposed llvm_unreachable was because it passes the tests and prevents future developer from depending on the code that depend on it assuming it works. | |
| clang/include/clang/AST/APValue.h | ||
|---|---|---|
| 620 | ReserveVector | |
| 627 | SetLValueEmptyPath | |
| clang/lib/AST/APValue.cpp | ||
| 599 | Yeah, I doubt we have good test coverage for all the various behaviors here. I was wondering if the qualifiers bit was handled properly with a simple cast. @rsmith is a good person to weigh in. | |
| clang/test/ASTMerge/APValue/APValue.cpp | ||
| 29 | We typically only use llvm_unreachable for situations where we believe the code path is impossible to reach, which is why I think that's the wrong approach. We could use an assertion to test the theory, however. | |
| clang/lib/AST/Expr.cpp | ||
|---|---|---|
| 325 | Can you use llvm_unreachable here? (Are there cases where we use RSK_None and then later find we actually have a value to store into the ConstantExpr?) | |
| clang/lib/Serialization/ASTReader.cpp | ||
| 9635 | This is problematic. ReadExpr will read a new copy of the expression, creating a distinct object. But in the case where we reach this when deserializing (for a MaterializeTemporaryExpr), we need to refer to the existing MaterializeTemporaryExpr in the initializer of its lifetime-extending declaration. We will also need to serialize the ASTContext's MaterializedTemporaryValues collection so that the temporaries lifetime-extended in a constant initializer get properly handled. That all sounds very messy, so I think we should reconsider the model that we use for lifetime-extended materialized temporaries. As a half-baked idea:
With that done, we should verify that all remaining Expr*s used as LValueBases are either only transiently used during evaluation or don't have these kinds of identity problems. | |
update done tasks.
| clang/lib/AST/APValue.cpp | ||
|---|---|---|
| 599 | the original question we had is whether it is correct to replace Ctx.ASTContext::getAsArrayType(ElemTy) by cast<ArrayType>(ElemTy.getCanonicalType()) in this context and the other comment below. | |
| clang/lib/AST/Expr.cpp | ||
| 325 | we can put llvm_unreachable in the switch because of if (!Value.hasValue()) above the switch but we can't remove if (!Value.hasValue()). | |
| clang/lib/Serialization/ASTReader.cpp | ||
| 9635 | Would it be possible to adapt serialization/deserialization so that they make sure that MaterializeTemporaryExpr are unique.
This would make easier adding new Expr that require uniqueness and seem less complicated. | |
| clang/lib/Serialization/ASTReader.cpp | ||
|---|---|---|
| 9635 | i added a review that does the refactoring https://reviews.llvm.org/D69360. | |
now that the issue with uniqueness of expressions is solved. we should be able to keep going on that review @rsmith.
https://reviews.llvm.org/D63960 should be i think close to completion. so maybe for testing i could use immediate invocation as a source for ConstantExpr instead of the code i added to make constexpr variables emit ConstantExpr ?