On Fri, Oct 17, 2014 at 4:49 PM, Zachary Turner <[email protected]> wrote: > In that case, can this be a follow-up patch (which may not happen > immediately)? It's using the A functions before my patch anyway, so I'm > introducing anything that's worse than before. I can leave a comment in the > code that mentions that we need to eventually convert to using W functions, > and doing so requires refactoring the registry support code.
I think that's reasonable -- this is still a step forward. However, it would be good to open up a bug against this. Paths are configurable for the SDK and toolchain installations, so it's perfectly reasonable for a user to have these installed on a path with Unicode characters. In those cases, we will fail hard. ~Aaron > > On Fri, Oct 17, 2014 at 1:43 PM, Aaron Ballman <[email protected]> > wrote: >> >> On Fri, Oct 17, 2014 at 4:38 PM, Zachary Turner <[email protected]> >> wrote: >> > >> > >> > On Fri, Oct 17, 2014 at 1:27 PM, Aaron Ballman <[email protected]> >> > wrote: >> >> >> >> On Fri, Oct 17, 2014 at 4:22 PM, Zachary Turner <[email protected]> >> >> wrote: >> >> > >> >> > >> >> > On Fri, Oct 17, 2014 at 12:48 PM, Aaron Ballman >> >> > <[email protected]> >> >> > wrote: >> >> >> >> >> >> On Fri, Oct 17, 2014 at 3:06 PM, Zachary Turner <[email protected]> >> >> >> wrote: >> >> >> > - >> >> >> > std::unique_ptr<Command> visualstudio::Compile::GetCommand( >> >> >> > Compilation &C, const JobAction &JA, const InputInfo &Output, >> >> >> > const InputInfoList &Inputs, const ArgList &Args, >> >> >> > Index: lib/Driver/WindowsToolChain.cpp >> >> >> > >> >> >> > =================================================================== >> >> >> > --- lib/Driver/WindowsToolChain.cpp >> >> >> > +++ lib/Driver/WindowsToolChain.cpp >> >> >> > @@ -77,61 +77,59 @@ >> >> >> > return getArch() == llvm::Triple::x86_64; >> >> >> > } >> >> >> > >> >> >> > +#ifdef USE_WIN32 >> >> >> > +static bool readFullStringValue(HKEY hkey, const char *valueName, >> >> >> > + std::string &value) { >> >> >> >> >> >> We should be preferring the W versions of these APIs instead of the >> >> >> A >> >> >> versions, especially since this is being used to pull out file >> >> >> paths. >> >> > >> >> > How does this work, since ultimately all of clang uses non-wide >> >> > character >> >> > strings anyway. I mean I know how to convert between the two, but I >> >> > was >> >> > under the impression that everything was just already broken because >> >> > afaik >> >> > we don't ever use W functions anywhere else. >> >> >> >> My understanding is that we use the W versions of the APIs and >> >> immediately convert to UTF-8 to store internally. When we require >> >> interaction in the other direction, we convert back to UTF-16. At >> >> least, this is how we work with things like command line arguments and >> >> files. As an example, see Process::GetArgumentVector. >> > >> > >> > So llvm::sys::windows::UTF8ToUTF16 and its counterpart are not exposed >> > in a >> > public header. Is there an accepted way to re-use them here, or do I >> > need >> > to duplicate the code in clang? >> >> There's not an accepted way currently, but duplication isn't the answer >> either. >> >> I think the registry code should be pulled down into an LLVM Support >> interface that's available on Windows (since we're already using >> USE_WIN32 within clang, I don't think the interface needs x-platform >> stubs), and the conversion routines hoisted to a place where they can >> be exposed for use within Support so that they can be used with the >> registry code. Registry use shouldn't be overly widespread, so I think >> the abstraction could be fairly simplistic. >> >> ~Aaron > > _______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
