Quuxplusone / LLVMBugzillaTest

0 stars 0 forks source link

incorrect logic when detect windows SDK #28379

Open Quuxplusone opened 8 years ago

Quuxplusone commented 8 years ago
Bugzilla Link PR28380
Status NEW
Importance P normal
Reported by comicfans44 (comicfans44@gmail.com)
Reported on 2016-07-01 03:31:34 -0700
Last modified on 2016-07-01 18:42:02 -0700
Version trunk
Hardware PC Windows NT
CC david.majnemer@gmail.com, llvm-bugs@lists.llvm.org, zturner@google.com
Fixed by commit(s)
Attachments 0001-Driver-fix-windows-SDK-detect.patch (2431 bytes, application/octet-stream)
Blocks
Blocked by
See also
Created attachment 16669
misc patch to address this

I've found clang version

 git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@274110 91177308-0d34-0410-b5e6-96231b3b80d8

counld not detect windows SDK, attached patch address this.

1. readFullStringValue returns bool, should not compared with ERROR_SUCCESS
2. RegQueryValueExW string may contain null terminating char ,should be trimmed
these leads getSystemRegistryString return incorrect result, and following
getWindows10SDKVersion gives incorrect result for string append, prevents
correct
SDK detected.
Quuxplusone commented 8 years ago

Attached 0001-Driver-fix-windows-SDK-detect.patch (2431 bytes, application/octet-stream): misc patch to address this

Quuxplusone commented 8 years ago

Zach, can you take a look? It seems you made the last major alterations to this code.

Quuxplusone commented 8 years ago

The bug tracker isn't really the appropriate place to review patches, could you upload this patch to Phabricator?

That said, it's fairly straightforward and my only comment is to use wstring::pop_back() instead of manually resizing it.

Quuxplusone commented 8 years ago
(In reply to comment #2)
> The bug tracker isn't really the appropriate place to review patches, could
> you upload this patch to Phabricator?
>
> That said, it's fairly straightforward and my only comment is to use
> wstring::pop_back() instead of manually resizing it.

I've created a review here:
http://reviews.llvm.org/D21946