[PATCH] D121510: [Support] Introduce the BLAKE3 hashing function implementation

[Duncan P. N. Exon Smith via Phabricator via llvm-commits]

dexonsmith added inline comments.


================
Comment at: llvm/include/llvm-c/blake3.h:41
+
+const char *blake3_version(void);
+void blake3_hasher_init(blake3_hasher *self);
----------------
akyrtzi wrote:
> lattner wrote:
> > How is this going to compose with apps that use LLVM and also use blake3 for their own things?  can we really take these global symbol names?  You'll note that in regex_impl.h we had to prefix the C symbols, I expect that we'll have the same challenges here.
> The original version of the patch had prefixes to avoid conflicts with clients having their own versions. The list of the changes in the original version are in the description, for reference:
> 
> `blake.h`:
> * Changes to avoid conflicts if a client also links with its own BLAKE3 version:
>   * Renamed the header macro guard with `LLVM_C_` prefix
>   * Renamed the C symbols to add the `llvm_` prefix
>   * Added a top header comment that references the CC0 license
> `blake3_impl.h`: Added `#define`s to remove some of `llvm_` prefixes for the rest of the internal implementation.
> Implementation files:
> * Used `llvm_` prefix for the C public API functions
> * Used `LLVM_LIBRARY_VISIBILITY` for internal implementation functions
> * Added `.private_extern`/`.hidden` in assembly files to reduce visibility of the internal implementation functions
> 
> During review @rnk [suggested](https://reviews.llvm.org/D121510#3383116) to land this with two patches:
> > What do you think about landing this as two patches, one which is a pristine source import, and the second which applies all the llvm_ prefixing changes, which I agree are necessary? This may make it easier to merge in future updates, should there ever be any.
> 
> Per the suggestion I modified the patch to keep the originating BLAKE3 sources pristine, with the intention to have a follow-up patch with LLVM-specific changes.
> 
> Does this make sense? Should I go back to the patch that embeds LLVM-specific changes, or do you have another suggestion?
I suggest:
- For this (the Phabricator review), squash into a single commit as it was before.
- Locally, keep it as two commits:
    1. `git add` for all the C implementation / README in the right place.
    2. Everything else (delete README, add license headers, add C++ wrapper, fix namespacing, etc.).
- Push both commits at once.

That makes it clear about what is being LGTM'ed here, while also providing the update feature @rnk was looking for.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D121510/new/

https://reviews.llvm.org/D121510