arames marked 2 inline comments as done.
arames added inline comments.
================
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) {
----------------
dexonsmith wrote:
> 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...
I created https://reviews.llvm.org/D109024 for this.
If we decide to go for that mechanism, whichever PR lands first, I can update
the other to use the new mechanism.
================
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 ?
----------------
dexonsmith wrote:
> I'd simplify:
> ```
> lang=c++
> // FIXME: Consider using SHA1 instead of MD5.
> ```
Do you think we should switch to `SHA1` now ?
I picked MD5 at random amongst the available hashes.
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
