This is an archive of the discontinued LLVM Phabricator instance.

[Windows] Sink function bodies to .cpp file
ClosedPublic

Authored by rnk on Nov 6 2018, 1:48 PM.

Details

Summary

These functions don't need to be inlined. I randomly picked Process.inc
for the Windows version helpers, since that's the most related file, and
Program.inc for MakeErrMsg since it's the main client.

While I'm here, move them into the llvm namespace, and delete the scoped
handle copy and assignment operators.

Event Timeline

rnk created this revision.Nov 6 2018, 1:48 PM
aganea added inline comments.Nov 6 2018, 2:15 PM
llvm/lib/Support/Windows/Process.inc
469

Can't you just replace the contents of this function by:

return GetWindowsOSVersion() >= llvm::VersionTuple(6, 2, 0, 0);
503

I'm wondering after all if we shouldn't just drop the part below? It's deprecated anyway, and the call to RtlGetVersion would probably always succeed (if you can't aquire a handle to ntdll.dll most likely you won't be able to run anything on your machine). What do you think?

rnk added inline comments.Nov 6 2018, 3:04 PM
llvm/lib/Support/Windows/Process.inc
469

Sounds good to me.

503

I'd be OK with that. I suppose we'd return VersionTuple(0,0,0,0) in that case, and we'd assume that flush file buffers was necessary, which is probably fine.

rnk updated this revision to Diff 172862.Nov 6 2018, 3:07 PM
  • simplify
aganea added inline comments.Nov 6 2018, 3:13 PM
llvm/lib/Support/Windows/Process.inc
485

It should say >= here.

rnk updated this revision to Diff 172865.Nov 6 2018, 3:16 PM
  • Fix thinko
aganea accepted this revision.Nov 6 2018, 3:24 PM

LGTM!

This revision is now accepted and ready to land.Nov 6 2018, 3:24 PM
This revision was automatically updated to reflect the committed changes.
aganea added a comment.Nov 6 2018, 4:51 PM

I was also wondering - there are a lot of interesting things in /lldb/trunk/source/Host/windows/. The ´GetWindowsOSVersion’ comes from there. What was the rationale for not having the ‘Host’ folder inside llvm? Any plans to move it there?

rnk added a comment.Nov 6 2018, 5:12 PM

Traditionally, LLDB has resisted efforts to more tightly couple it with LLVM. Many of the contributors at Apple seem to prefer to build LLVM separately and then build against that with the LLDB xcode project. You can see how that would disincentivize you from contributing to LLVM first. And in fairness, sometimes LLVM people object to adding extra complexity to Support when it's only used in LLDB. And, as with lib/DebugInfo/DWARF, sometimes code gets hoisted to LLVM without refactoring LLDB to use it. So, there's plenty of work to go around. :)