> > + auto const level_to_string = [](spv_message_level_t level){
>
> "const auto" would be more idiosyncratic.
>
> > + switch (level) {
> > + case SPV_MSG_FATAL:
> > + return std::string("Fatal");
> > + case SPV_MSG_INTERNAL_ERROR:
> > + return std::string("Internal error");
> > + case SPV_MSG_ERROR:
> > + return std::string("Error");
> > + case SPV_MSG_WARNING:
> > + return std::string("Warning");
> > + case SPV_MSG_INFO:
> > + return std::string("Info");
> > + case SPV_MSG_DEBUG:
> > + return std::string("Debug");
> > + }
> > + return std::string();
> > + };
>
> You could just use a 'level == X ? "Y" : ...' ladder here instead since
> the 'level' argument is already known at the point the level_to_string
> function is defined.
Karol took the switch out of the lambda function and removed the lambda; I
can’t remember why I had it that.
> > +bool
> > +clover::spirv::is_binary_spirv(const char *il, size_t length)
> > +{
> > + const uint32_t *binary = reinterpret_cast<const uint32_t*>(il);
> > +
> > + // A SPIR-V binary is at the very least 5 32-bit words, which represent
> > the
> > + // SPIR-V header.
> > + if (length < 20u)
> > + return false;
> > +
> > + const uint32_t first_word = binary[0u];
> > + return (first_word == SpvMagicNumber) ||
> > + (util_bswap32(first_word) == SpvMagicNumber);
> > +}
> > +
>
> This function seems like dead code. Maybe drop it? Then you'll be able
> to use a single #ifdef preprocessor conditional to guard the whole file.
The code using it was moved to a separate MR which will be sent after this one;
the function has now been moved to the later MR.
>
> > +#ifdef CLOVER_ALLOW_SPIRV
> > +bool
> > +clover::spirv::is_valid_spirv(const uint32_t *binary, size_t length,
> > + const std::string &opencl_version,
> > + const context::notify_action ¬ify) {
>
> We don't need an extra level of call-backs here, you can pass a
> 'std::string &r_log' reference and append the errors there, just like
> the LLVM codepaths do too return error strings.
Would this work better for you?
```
bool
clover::spirv::is_valid_spirv(const uint32_t *binary, size_t length,
const std::string &opencl_version,
std::string &r_log) {
auto const validator_consumer =
[¬ify](spv_message_level_t level, const char *source,
const spv_position_t &position, const char *message) {
r_log += format_validator_msg(level, source, position,
message);
};
const spv_target_env target_env =
convert_opencl_str_to_target_env(opencl_version);
spvtools::SpirvTools spvTool(target_env);
spvTool.SetMessageConsumer(validator_consumer);
return spvTool.Validate(binary, length);
}
```
And then it would be the caller’s responsibility to get the string back to the
user.
> Other than that looks good:
>
> Reviewed-by: Francisco Jerez <curroje...@riseup.net>
Thank you for the review!
Pierre
signature.asc
Description: PGP signature
_______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
