dexonsmith added a comment.
A couple of suggestions inline, but this basically LGTM.
@jansvoboda11 @Bigcheese : can you take a look as well and confirm this looks
reasonable from your perspectives?
================
Comment at: clang/include/clang/Basic/ObjCRuntime.h:486
+ template <typename HasherT, llvm::support::endianness Endianness>
+ friend void addHash(llvm::HashBuilderImpl<HasherT, Endianness> &HBuilder,
+ const ObjCRuntime &OCR) {
----------------
arames wrote:
> I have added these in the same line as existing `hash_value` functions. The
> idea being others may also need to hash those objects.
> Let me know if you would rather move some/all of these locally in
> `CompilerInvocation.cpp`.
Seems reasonable to me.
An option to consider (maybe in a separate commit?) would be to add:
```
lang=c++
class HashCodeHasher {
public:
void update(ArrayRef<uint8_t> Bytes) {
llvm::hash_code BytesCode = llvm::hash_value(Bytes);
Code = Code ? BytesCode : llvm::hash_combine(*Code, BytesCode);
}
Optional<llvm::hash_code> Code;
};
template <class T,
std::enable_if_t<is_hashable<T>::value, bool> = true>
llvm::hash_code hash_value(const T &Value) {
HashBuilder<HashCodeHasher, support::endianness::native> Builder;
Builder.add(Value);
return *Builder.Code;
}
```
Then objects that support HashBuilder also support `llvm::hash_value` without
extra code...
================
Comment at: clang/lib/Frontend/CompilerInvocation.cpp:4473-4475
+ // FIXME: We want to use something cryptographically sound. This was
+ // initially using CityHash (via `llvm::hash_code`). We moved to `llvm::MD5`.
+ // Is thie appropriate ?
----------------
I'd simplify:
```
lang=c++
// FIXME: Consider using SHA1 instead of MD5.
```
================
Comment at: llvm/include/llvm/Support/HashBuilder.h:167
/// template <typename HasherT, support::endianness Endianness>
- /// void addHash(HashBuilder<HasherT, Endianness> &HBuilder,
+ /// void addHash(HashBuilderImpl<HasherT, Endianness> &HBuilder,
/// const UserDefinedStruct &Value);
----------------
Please fix in a separate commit (no need for a review IMO).
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D102943/new/
https://reviews.llvm.org/D102943
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
