[Compiler]: Using Located<T> instead of std::pair<SourceLoc, T> #28643
Conversation
| friend bool operator ==(const Located<U> lhs, const Located<U> rhs) { | ||
| return lhs.item == rhs.item && lhs.loc == rhs.loc; | ||
| } | ||
| }; |
There was a problem hiding this comment.
Could you add a dump method that does something reasonable like for other types? You could have two overloads: void dump() const; void dump(llvm::raw_ostream &OS) const where the former calls the latter using llvm::errs(). [For example, SourceFile does this.]
You could add a new file Located.cpp which has the implementations and the appropriate #includes. You will also need to add Located.cpp to the lib/Basic/CMakeLists.txt file.
There was a problem hiding this comment.
@kitaisreal what do you think about this point? I think it might be useful to have this method for debugging.
As to what is should print, perhaps something that prints the SourceLoc first and then the item, leaving a space in between (say)
There was a problem hiding this comment.
@varungandhi-apple I agree, that we should introduce such function. Added.
|
Thanks for the PR! I've made some suggestions for changes. The @swift-ci please smoke test |
|
Updated for pairs like std::pair<SourceLoc, ...> |
|
@swift-ci please smoke test |
|
Sorry, I didn't pick this up earlier, but I think we should have the field names in |
|
@varungandhi-apple Changed Located field names in PascalCase, Item and Loc. |
|
@kitaisreal there are some merge conflicts. So you might want to rebase on top of latest When you rebase, you'll pick up a patch I got merged recently (commit: fb59293), which introduces a struct |
kitaisreal
force-pushed
the
using-located-instead-of-pair
branch
from
45ba939 to
e89e2dc
Compare
|
@varungandhi-apple fixed conflicts, also updated Convention to use Located. |
|
Awesome. I'm traveling for the next ~2 days, I'll get back to reviewing after that. @swift-ci please smoke test |
|
@varungandhi-apple currently the build from utils/build-script is passing without any errors. But on CI we are trying to build a toolchain and it is failing in lldb ? Can I reproduce such build error without trying to build a toolchain ? |
|
You can use |
include/swift/Basic/Located.h
Outdated
| /// The original source location from which the item was parsed. | ||
| SourceLoc Loc; | ||
|
|
||
| Located(): Item(), Loc() {} |
There was a problem hiding this comment.
Hm, I wonder if it makes sense to have this constructor given that not every T can be default or value-initialized. It might make more sense to provide an explicit constructor from a T or const T&, or default the loc in the other constructor and make it explicit.
There was a problem hiding this comment.
If this is the route you take, please also define an explicit move constructor.
There was a problem hiding this comment.
I think the reason for adding this was that DenseMap required this constructor for its values?
There was a problem hiding this comment.
We should give a DenseMapInfo specialization to avoid that...
There was a problem hiding this comment.
Hi, @CodaFi
As I understand correctly from code base we are making such specializations for Key:
namespace llvm {
template <typename T> struct DenseMapInfo;
template<typename T>
struct DenseMapInfo<swift::Located<T>> {
static inline swift::Located<T> getEmptyKey() {
return swift::Located<T>(T(), swift::SourceLoc());
}
static inline swift::Located<T> getTombstoneKey() {
return swift::Located<T>(T(), swift::SourceLoc());
}
static unsigned getHashValue(const swift::Located<T> &LocatedVal) {
return combineHashValue(DenseMapInfo<T>::getHashValue(LocatedVal.Item),
DenseMapInfo<swift::SourceLoc>::getHashValue(LocatedVal.Loc));
}
static bool isEqual(const swift::Located<T> &LHS, const swift::Located<T> &RHS) {
return LHS == RHS;
}
};
} // namespace llvmBut problem in DenseMap
value_type& FindAndConstruct(const KeyT &Key) {
BucketT *TheBucket;
if (LookupBucketFor(Key, TheBucket))
return *TheBucket;
return *InsertIntoBucket(TheBucket, Key);
}
ValueT &operator[](const KeyT &Key) {
return FindAndConstruct(Key).second /**here**/;
}As I understand it tries to default initialize Value. And solution with DenseMapInfo specialization will not help in such case, where Located is Value. Is it right ?
There was a problem hiding this comment.
Tombstone and empty key requirements are not correctly implemented. Take a look at how LLVM implements this for std::pair to get an idea of how this should look. You need to delegate to the DenseMapInfo of T and the SourceLoc.
include/swift/Basic/Located.h
Outdated
| template<typename U> | ||
| friend bool operator ==(const Located<U> lhs, const Located<U> rhs) { | ||
| friend bool operator ==(const Located<U>& lhs, const Located<U>& rhs) { |
There was a problem hiding this comment.
@CodaFi's comment was probably about this line, which should have const Located<U> &lhs instead of const Located<U>& lhs and similarly for rhs.
|
Mostly LGTM but you'll need to resolve the merge conflicts and address the failing LLDB compilation. Feel free to @ me or Robert (CodaFi) to run the tests. |
|
@swift-ci please test |
1 similar comment
|
@swift-ci please test |
|
Build failed |
|
Build failed |
|
Hi, @CodaFi @varungandhi-apple I was able to successfully build lldb locally, but small changes were introduced in this repository https://github.com/apple/llvm-project. |
|
Branch from |
|
@CodaFi, @varungandhi-apple created pull request swiftlang/llvm-project#522. |
|
@swift-ci please test |
|
Build failed |
|
Build failed |
|
@kitaisreal can you look at the test failures? It looks like |
|
@varungandhi-apple Sure. Updated. |
|
@swift-ci please test |
|
Build failed |
|
Build failed |
|
I think that this may have caused a build break for the Windows builds: |
|
Additional breakage here: https://ci-external.swift.org/job/oss-swift-windows-x86_64/2363/ |
|
@compnerd yes, this is related. I merged it after the tests were passing for Linux and macOS. Is this an MSVC bug? Don't understand why it is complaining about a duplicate, there is exactly one |
|
I dion't think that this is a bug at all. I think that the compiler is identifying an issue with the code. The warning is documented here https://docs.microsoft.com/en-us/cpp/error-messages/compiler-errors-2/compiler-error-c2995?view=vs-2019 . The template is getting instantiated multiple times. I think I have a fix for this, I'll push up a change. |
|
See #29032 |
|
I tried compiling this locally and I saw the same error with Clang. Wondering how it got through in CI. 😕 |
|
This fails to build on Fedora 31 (x86_64) and I'm surprised this builds at all. I can't find a declaration or definition of |
|
There are two fundamental problems here:
I'm just going to comment the problematic line out, since older clangs don't emit the function anyway and @varungandhi-apple seemingly did not notice. See #29045 |
https://bugs.swift.org/browse/SR-11889
@varungandhi-apple