[PATCH] D128560: An upcoming patch to LLDB will require the ability to decode base64. This patch adds support for decoding base64 and adds tests.

[David Blaikie via Phabricator via llvm-commits]

dblaikie added a comment.

In D128560#3681043 <https://reviews.llvm.org/D128560#3681043>, @clayborg wrote:

> In D128560#3680975 <https://reviews.llvm.org/D128560#3680975>, @aprantl wrote:
>
>> For compilation speed I would prefer sinking decodeBase64Byte() into a .cpp file and I would probably also prefer to move the implementation of decodeBase64() even if that means it can no longer be a template and we would need to require i.e., a SmallVectorImpl or something like that. Are there any really big advantages of making this a template?
>
> I agree as I was wondering if decodeBase64Byte would cause problems being a static in the header file.

Outside of commentary on the rest of the patch/design, I can say that making a function namespace (not class) scoped static in a header is a problem, as it leads to ODR violations in any inline function that calls the static one (since every definition of an inline function must be identical, including it must refer to the same entities by name - but the static function is distinct in each TU, so the inline functions refer to different functions (though they have the same name) - so ODR violation). But a direct fix for that in this case would be to make the function inline (think of inline as a linkage, not as telling the compiler to inline the function (it does bias the inliner slightly, but not a big deal) - "inline" just means "let me define this repeatedly, I guarantee all the definitions will be the same"), rather than static.

But yeah, probably moving the definition to the .cpp file is more suitable here in any case.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128560