[PATCH] D133716: [CAS] Add LLVMCAS library with InMemoryCAS implementation

[Ben Langmuir via Phabricator via llvm-commits]

benlangmuir added inline comments.


================
Comment at: llvm/include/llvm/CAS/ObjectStore.h:59
+///   "Loading" the object is a separate step that may not have happened
+///   yet, and which can fail (e.g. due to filesystem corruption) or introduce
+///   latency (if downloading from a remote store).
----------------
steven_wu wrote:
> dblaikie wrote:
> > Presumably it can also fail if the object isn't in the given CAS? (maybe that's the more obvious/simpler to document example?)
> This question is complicated. Maybe we should write down a spec for the CAS APIs? To this question, the current spec for when to lookup for a CAS object to make sure it exists is totally implementation defined.
> 
> For builtinCAS here, ObjectRef always points to existing object, unless the integrity of the builtinCAS is broken. For a remote CAS, you might actually want to have ObjectRef to be unverified to avoid a roundtrip to remote to validate a ObjectRef. 
> 
> Also to you previous point of split up public/private interface document, is it better if I create a separate docs in `llvm/docs` to explain different concepts from the views of: 1. CAS users 2. CAS implementors?
> This question is complicated. Maybe we should write down a spec for the CAS APIs? To this question, the current spec for when to lookup for a CAS object to make sure it exists is totally implementation defined

This complication is only for the implementation though, right? Clients of the object store shouldn't assume the object exists in the CAS, and like @dblaikie said, loading can fail if it is not.  I don't think we want to promise that builtin CAS will always succeed when loading, since we might want to change that kind of detail later.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133716