[PATCH] D131992: [Support] compression proposal for a enum->spec->impl approach

MaskRay added a comment. (I was out of town last week so I did not respond then.) In D131992#3748306 , @dblaikie wrote: > @MaskRay I think this change is probably the best point of > comparison/demonstration of this patch direction (taking some minor li

[PATCH] D131992: [Support] compression proposal for a enum->spec->impl approach

dblaikie added a comment. @MaskRay I think this change is probably the best point of comparison/demonstration of this patch direction (taking some minor liberties with this patch to simplify things somewhat in ways that have already been discussed/seem quite reasonable - specifically having `ge

[PATCH] D131992: [Support] compression proposal for a enum->spec->impl approach

MaskRay added a comment. Both D131638 and this patch want to use classes with inheritance to support different compression algorithms. There have been many versions but I think I have never received a convincing argument how such an inheritance based design loo

[PATCH] D131992: [Support] compression proposal for a enum->spec->impl approach

ckissane marked 2 inline comments as done. ckissane added inline comments. Comment at: llvm/include/llvm/Support/Compression.h:38-39 + +CompressionSpecRef getCompressionSpec(uint8_t Kind); +CompressionSpecRef getCompressionSpec(CompressionKind Kind); +CompressionSpecRef getScheme

[PATCH] D131992: [Support] compression proposal for a enum->spec->impl approach

ckissane updated this revision to Diff 453803. ckissane added a comment. - shorten compression related variable names Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D131992/new/ https://reviews.llvm.org/D131992 Files: clang-tools-extra/clangd/inde

[PATCH] D131992: [Support] compression proposal for a enum->spec->impl approach

dblaikie added inline comments. Comment at: llvm/include/llvm/Object/Decompressor.h:52-53 uint64_t DecompressedSize; + compression::CompressionSpecRef CompressionScheme = + compression::CompressionSpecRefs::Zlib; }; ckissane wrote: > dblaikie wrote: > >

[PATCH] D131992: [Support] compression proposal for a enum->spec->impl approach

ckissane marked an inline comment as done. ckissane added a comment. @dblaikie I've handled most of your comments, can you take another look? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D131992/new/ https://reviews.llvm.org/D131992 __

[PATCH] D131992: [Support] compression proposal for a enum->spec->impl approach

ckissane updated this revision to Diff 453774. ckissane marked an inline comment as done. ckissane added a comment. - CompressionImplementation member on Decompressor instead of Spec Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D131992/new/ https:/

[PATCH] D131992: [Support] compression proposal for a enum->spec->impl approach

ckissane marked an inline comment as done. ckissane added inline comments. Comment at: llvm/include/llvm/Object/Decompressor.h:52-53 uint64_t DecompressedSize; + compression::CompressionSpecRef CompressionScheme = + compression::CompressionSpecRefs::Zlib; }; ---

[PATCH] D131992: [Support] compression proposal for a enum->spec->impl approach

ckissane updated this revision to Diff 453723. ckissane added a comment. - fix static initialization skip errors in getCompressionSpec Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D131992/new/ https://reviews.llvm.org/D131992 Files: clang-tools-

[PATCH] D131992: [Support] compression proposal for a enum->spec->impl approach

ckissane updated this revision to Diff 453409. ckissane added a comment. - small compression test cleanup Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D131992/new/ https://reviews.llvm.org/D131992 Files: clang-tools-extra/clangd/index/Serializat

[PATCH] D131992: [Support] compression proposal for a enum->spec->impl approach

ckissane updated this revision to Diff 453404. ckissane marked 3 inline comments as done. ckissane added a comment. - remove Compression{Spec,Impl}Ref typedef Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D131992/new/ https://reviews.llvm.org/D13199

[PATCH] D131992: [Support] compression proposal for a enum->spec->impl approach

ckissane marked 6 inline comments as done. ckissane added inline comments. Comment at: llvm/include/llvm/Support/Compression.h:40 +CompressionSpecRef getCompressionSpec(CompressionKind Kind); +CompressionSpecRef getSchemeDetails(CompressionImplRef Implementation); + -

[PATCH] D131992: [Support] compression proposal for a enum->spec->impl approach

ckissane updated this revision to Diff 453379. ckissane added a comment. - remove CompressionSpecRefs::{Zlib,ZStd} Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D131992/new/ https://reviews.llvm.org/D131992 Files: clang-tools-extra/clangd/index/S

[PATCH] D131992: [Support] compression proposal for a enum->spec->impl approach

ckissane added inline comments. Comment at: llvm/include/llvm/Support/Compression.h:95-98 + static CompressionSpecRef Unknown; + static CompressionSpecRef None; + static CompressionSpecRef Zlib; + static CompressionSpecRef ZStd; dblaikie wrote: > ckissane wro

[PATCH] D131992: [Support] compression proposal for a enum->spec->impl approach

dblaikie added a comment. (still a fair few unhandled comments from the last round - guess work to address those is still ongoing) Comment at: llvm/include/llvm/Support/Compression.h:95-98 + static CompressionSpecRef Unknown; + static CompressionSpecRef None; + static Compr

[PATCH] D131992: [Support] compression proposal for a enum->spec->impl approach

ckissane updated this revision to Diff 453143. ckissane added a comment. - remove extra includes of ADT/Optional Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D131992/new/ https://reviews.llvm.org/D131992 Files: clang-tools-extra/clangd/index/Ser

[PATCH] D131992: [Support] compression proposal for a enum->spec->impl approach

ckissane added inline comments. Comment at: llvm/include/llvm/Support/Compression.h:95-98 + static CompressionSpecRef Unknown; + static CompressionSpecRef None; + static CompressionSpecRef Zlib; + static CompressionSpecRef ZStd; dblaikie wrote: > Generally we

[PATCH] D131992: [Support] compression proposal for a enum->spec->impl approach

ckissane updated this revision to Diff 453140. ckissane added a comment. - compression: remove some usage sugar from Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D131992/new/ https://reviews.llvm.org/D131992 Files: clang-tools-extra/clangd/index

[PATCH] D131992: [Support] compression proposal for a enum->spec->impl approach

ckissane marked an inline comment as done. ckissane added inline comments. Comment at: llvm/include/llvm/Support/Compression.h:48-50 + const int BestSpeedLevel; + const int DefaultLevel; + const int BestSizeLevel; dblaikie wrote: > These could/should probably

[PATCH] D131992: [Support] compression proposal for a enum->spec->impl approach

ckissane updated this revision to Diff 453139. ckissane added a comment. - move compression level members from spec to impl Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D131992/new/ https://reviews.llvm.org/D131992 Files: clang-tools-extra/clang

[PATCH] D131992: [Support] compression proposal for a enum->spec->impl approach

ckissane updated this revision to Diff 453136. ckissane added a comment. - remove Supported member of CompressionSpec Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D131992/new/ https://reviews.llvm.org/D131992 Files: clang-tools-extra/clangd/inde

[PATCH] D131992: [Support] compression proposal for a enum->spec->impl approach

ckissane added inline comments. Comment at: llvm/lib/Object/Decompressor.cpp:50 +return createError( +"Decompressor provided nullptr (None) CompressionScheme*"); + if (!CompressionScheme->Implementation) dblaikie wrote: > This probably isn't a useful

[PATCH] D131992: [Support] compression proposal for a enum->spec->impl approach

dblaikie added a comment. Probably easier to review if it's framed somewhat more like @MaskRay and my examples - backwards compatible, without fixing all the API users as we can discuss those separately. Though it is useful to have one or two example uses - but having all the API uses being up

[PATCH] D131992: [Support] compression proposal for a enum->spec->impl approach

ckissane created this revision. ckissane added reviewers: dblaikie, MaskRay. Herald added subscribers: StephenFan, wenlei, kadircet, arphaman, hiraditya, arichardson, emaste. Herald added a reviewer: alexander-shaposhnikov. Herald added a reviewer: rupprecht. Herald added a reviewer: jhenderson. H