This is an archive of the discontinued LLVM Phabricator instance.

Differential D53031

[LLD] [MinGW] Expand comment for MinGW driver.
ClosedPublic

Authored by mstorsjo on Oct 9 2018, 10:43 AM.

Details

Reviewers
ruiu
smeenai
Commits
rG5abe10ac2108: [MinGW] Expand comment for MinGW driver. NFC.
rLLD350464: [MinGW] Expand comment for MinGW driver. NFC.
rL350464: [MinGW] Expand comment for MinGW driver. NFC.
Summary

Expand comment for MinGW driver.

Diff Detail

Event Timeline

ruiu created this revision.Oct 9 2018, 10:43 AM
Harbormaster completed remote builds in B23595: Diff 168831.Oct 9 2018, 10:43 AM
ruiu updated this revision to Diff 168832.Oct 9 2018, 10:45 AM
  • fix typos
Harbormaster completed remote builds in B23596: Diff 168832.Oct 9 2018, 10:45 AM
smeenai added a subscriber: smeenai.Oct 9 2018, 11:17 AM
smeenai added inline comments.
lld/MinGW/Driver.cpp
10

Should be either "consists of" or "comes with"

16

differnet -> different

18

translate -> translates

21

calls lld/COFF driver -> calls the lld/COFF driver?

22

run lld/COFF linker -> run the lld/COFF linker?

26

implemented -> implements?

mstorsjo added a comment.Oct 9 2018, 11:40 AM

+1 to the rest of @smeenai's comments

lld/MinGW/Driver.cpp
26

I don't see anything wrong with what's written here at the moment.

smeenai added inline comments.Oct 9 2018, 11:43 AM
lld/MinGW/Driver.cpp
26

It sounds more natural in the present tense to me (it flows better from the previous part of the sentence), but yeah, it's fine either way.

mstorsjo added a comment.Oct 25 2018, 1:05 PM

@ruiu Do you have time to finish this one, there shouldn't be much to do?

mstorsjo commandeered this revision.Jan 4 2019, 2:27 PM
mstorsjo updated this revision to Diff 180324.
mstorsjo edited reviewers, added: ruiu; removed: mstorsjo.
mstorsjo retitled this revision from Expand comment for MinGW driver. to [LLD] [MinGW] Expand comment for MinGW driver..
mstorsjo added a reviewer: smeenai.

Corrected @smeenai's review comments.

ruiu accepted this revision.Jan 4 2019, 2:57 PM

LGTM

Feel free to commit.

This revision is now accepted and ready to land.Jan 4 2019, 2:57 PM
Closed by commit rL350464: [MinGW] Expand comment for MinGW driver. NFC. (authored by mstorsjo). · Explain WhyJan 5 2019, 2:47 AM
This revision was automatically updated to reflect the committed changes.

Revision Contents

PathSize
lld/
MinGW/
24 lines

Diff 168832

lld/MinGW/Driver.cpp

//===- MinGW/Driver.cpp ---------------------------------------------------===// //===- MinGW/Driver.cpp ---------------------------------------------------===//
// //
// The LLVM Linker // The LLVM Linker
// //
// This file is distributed under the University of Illinois Open Source // This file is distributed under the University of Illinois Open Source
// License. See LICENSE.TXT for details. // License. See LICENSE.TXT for details.
// //
//===----------------------------------------------------------------------===// //===----------------------------------------------------------------------===//
/// //
/// GNU ld style linker driver for COFF currently supporting mingw-w64. // MinGW is a GNU development environment for Windows. It consists with GNU
smeenaiUnsubmitted
Not Done
ReplyInline Actions

Should be either "consists of" or "comes with"

smeenai: Should be either "consists of" or "comes with"
/// // tools such as GCC and GNU ld. Unlike Cygwin, there's no POSIX-compatible
// layer, as it aims to be a native development toolchain.
//
// lld/MinGW is a drop-in replacement for GNU ld/MinGW.
//
// Being a native development tool, a MinGW linker is not very differnet from
smeenaiUnsubmitted
Not Done
ReplyInline Actions

differnet -> different

smeenai: differnet -> different
// Microsoft link.exe, so a MinGW linker can be implemented as a thin wrapper
// for lld/COFF. This driver takes Unix-ish command line options, translate them
smeenaiUnsubmitted
Not Done
ReplyInline Actions

translate -> translates

smeenai: translate -> translates
// to Windows-ish ones, and then passes them to lld/COFF.
//
// When this driver calls lld/COFF driver, it passes a hidden option "-lldmingw"
smeenaiUnsubmitted
Not Done
ReplyInline Actions

calls lld/COFF driver -> calls the lld/COFF driver?

smeenai: calls lld/COFF driver -> calls **the** lld/COFF driver?
// along with other user-supplied options, to run lld/COFF linker in "MinGW
smeenaiUnsubmitted
Not Done
ReplyInline Actions

run lld/COFF linker -> run the lld/COFF linker?

smeenai: run lld/COFF linker -> run **the** lld/COFF linker?
// mode".
//
// There are subtle differences between MS link.exe and GNU ld/MinGW, and GNU
// ld/MinGW implemented a few GNU-specific features. Such features are directly
smeenaiUnsubmitted
Not Done
ReplyInline Actions

implemented -> implements?

smeenai: implemented -> implements?
mstorsjoAuthorUnsubmitted
Not Done
ReplyInline Actions

I don't see anything wrong with what's written here at the moment.

mstorsjo: I don't see anything wrong with what's written here at the moment.
smeenaiUnsubmitted
Not Done
ReplyInline Actions

It sounds more natural in the present tense to me (it flows better from the previous part of the sentence), but yeah, it's fine either way.

smeenai: It sounds more natural in the present tense to me (it flows better from the previous part of…
// implemented in lld/COFF and enabled only when the linker is running in MinGW
// mode.
//
//===----------------------------------------------------------------------===// //===----------------------------------------------------------------------===//
#include "lld/Common/Driver.h" #include "lld/Common/Driver.h"
#include "lld/Common/ErrorHandler.h" #include "lld/Common/ErrorHandler.h"
#include "llvm/ADT/ArrayRef.h" #include "llvm/ADT/ArrayRef.h"
#include "llvm/ADT/Optional.h" #include "llvm/ADT/Optional.h"
#include "llvm/ADT/StringExtras.h" #include "llvm/ADT/StringExtras.h"
#include "llvm/ADT/StringRef.h" #include "llvm/ADT/StringRef.h"
▲ Show 20 Lines • Show All 242 Lines • Show Last 20 Lines