Vedran Miletić <ved...@miletic.net> writes: > OpenCL apps can quote arguments they pass to the OpenCL compiler, most > commonly include paths containing spaces. If the OpenCL compiler was > called via a shell, the shell would remove (single or double) quotes > before passing the argument to the compiler. Since we call Clang as a > library, we have to remove quotes before passing the argument.
Sigh, it's a shame that the OpenCL spec doesn't specify what format the option string should have, whether quotes have any special interpretation at all, whether escape sequences are supported, etc. Might be worth reporting the problem at Khronos -- Or have you found any evidence in the spec that the parsing logic you implement below is the correct one? > --- > .../state_trackers/clover/llvm/invocation.cpp | 41 > +++++++++++++++++++--- > 1 file changed, 36 insertions(+), 5 deletions(-) > > diff --git a/src/gallium/state_trackers/clover/llvm/invocation.cpp > b/src/gallium/state_trackers/clover/llvm/invocation.cpp > index e2cadda..d389a76 100644 > --- a/src/gallium/state_trackers/clover/llvm/invocation.cpp > +++ b/src/gallium/state_trackers/clover/llvm/invocation.cpp > @@ -147,12 +147,43 @@ namespace { > > // Parse the compiler options: > std::vector<std::string> opts_array; The code below is complex enough (and largely unrelated to the rest of compile_llvm()) that it would be worth putting it into a separate function -- How about 'vector<string> quoted_tokenize(const string &)' or something similar? > - std::istringstream ss(opts); > + std::ostringstream builder; > + > + // OpenCL programs can pass a single or double quoted argument, most > + // frequently include path. This is useful so that the path containing > + // spaces is treated as a single argument, but we should anyhow unquote > + // quoted arguments before passing them to the compiler. > + // We do not want to avoid using std::string::replace here, as include > + // path can contain quotes in file names. > + bool escape_next = false; > + bool skip_space = false; I believe this is just 'in_quote_single || in_quote_double'. I would feel slightly more confident of the logic below (one less bit of state mutated by induction across loop iterations) if you dropped this induction variable and wrote 'in_quote_single || in_quote_double' in closed form in the code below instead. > + bool in_quote_double = false; > + bool in_quote_single = false; > + for (auto pos = std::begin(opts); pos != std::end(opts); ++pos) { You could just do something like 'for (auto c : opts) {'. > + if (escape_next) { > + builder.put(*pos); > + escape_next = false; > + } else if (*pos == '\\') { > + escape_next = true; > + } else if (*pos == '"' && !in_quote_single) { > + in_quote_double = !in_quote_double; > + skip_space = !skip_space; > + } else if (*pos == '\'' && !in_quote_double) { > + in_quote_single = !in_quote_single; > + skip_space = !skip_space; > + } else if (*pos == ' ' && !skip_space && builder.tellp() > 0) { > + opts_array.emplace_back(builder.str()); > + builder.str(""); > + } else if (*pos != ' ' || skip_space) { > + builder.put(*pos); > + } Because the last two conditions are fully disjoint you could swap them and then simplify the last one, like: | } else if (*pos != ' ' || skip_space) { | builder.put(*pos); | } else if (builder.tellp() > 0) { | opts_array.emplace_back(builder.str()); | builder.str(""); | } (Note that by doing this you get rid of one of the two occurrences of skip_space which will be useful if you take my suggestion to drop it). > + } > + if (builder.tellp() > 0) { > + opts_array.emplace_back(builder.str()); > + } We usually omit redundant loop and conditional block braces when the body is a single statement. > > - while (!ss.eof()) { > - std::string opt; > - getline(ss, opt, ' '); > - opts_array.push_back(opt); > + if (in_quote_double || in_quote_single) { Same here. BTW, this gives me the impression we could have done the same thing in at most two lines of code by using boost::tokenizer -- We don't depend on Boost right now though so I guess I don't have any better ideas in mind than roughly what you have done here. > + throw error(CL_INVALID_COMPILER_OPTIONS); > } > > opts_array.push_back(name); > -- > 2.7.4 > > _______________________________________________ > mesa-dev mailing list > mesa-dev@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/mesa-dev
signature.asc
Description: PGP signature
_______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev