[PATCH] D137338: Fix dupe word typos

[Owen Pan via Phabricator via cfe-commits]

Herald added a comment.

NOTE: Clang-Format Team Automated Review Comment

It looks like your clang-format review does not contain any unit tests, please 
try to ensure all code changes have a unit test (unless this is an `NFC` or 
refactoring, adding documentation etc..)

Add your unit tests in `clang/unittests/Format` and you can build with `ninja 
FormatTests`.  We recommend using the `verifyFormat(xxx)` format of unit tests 
rather than `EXPECT_EQ` as this will ensure you change is tolerant to random 
whitespace changes (see FormatTest.cpp as an example)

For situations where your change is altering the TokenAnnotator.cpp which can 
happen if you are trying to improve the annotation phase to ensure we are 
correctly identifying the type of a token, please add a token annotator test in 
`TokenAnnotatorTest.cpp`




Comment at: clang/lib/Format/TokenAnnotator.cpp:4899-4902
+  // Note that this allows the "{" to go over the column limit
   // when the column limit is just between ":" and "{", but that does
   // not happen too often and alternative formattings in this case are
   // not much better.

We should reflow the comment:
```
  // Note that this allows the "{" to go over the column limit when the
  // column limit is just between ":" and "{", but that does not happen too
  // often and alternative formattings in this case are not much better.
```



Comment at: clang/lib/Format/WhitespaceManager.h:202-203
 
-// Determine if every row in the the array
+// Determine if every row in the array
 // has the same number of columns.
 bool isRectangular() const {

We should merge the comment into a single line.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D137338/new/

https://reviews.llvm.org/D137338

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D137338: Fix dupe word typos

[Louis Dionne via Phabricator via cfe-commits]

ldionne added a comment.
Herald added subscribers: wangpc, gysit, Dinistro, hoy, bviyer, wlei, jplehr, 
PiotrZSL, luke, sunshaoce.
Herald added a reviewer: springerm.
Herald added a reviewer: kiranchandramohan.
Herald added a project: clang-format.
Herald added reviewers: rymiel, HazardyKnusperkeks, owenpan, MyDeveloperDay.

The libcxx, libcxxabi and libunwind parts were committed in 
b397921fc7ef4e2882b3ae9c5f12fb40daca4078 
. Is it 
possible to close this or rebase it so we can remove the libc++ subscription to 
clear the review queue? We're trying to clean things up as part of the Github 
PR transition.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D137338/new/

https://reviews.llvm.org/D137338

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D137338: Fix dupe word typos

[Nico Weber via Phabricator via cfe-commits]

thakis added inline comments.
Herald added a reviewer: njames93.



Comment at: clang/lib/Driver/ToolChains/Darwin.cpp:47
   // complete architecture list, nor a reasonable subset. The problem is that
-  // historically the driver driver accepts this and also ties its -march=
   // handling to the architecture name, so we need to be careful before 
removing

aaron.ballman wrote:
> This is duplicated just often enough and in just enough contexts where I 
> think "driver driver" means "the driver that drives another driver", but it'd 
> be nice if someone else could confirm or deny that.
I think that's correct. IIRC, the "driver driver" was a feature in apple gcc 
long ago, and it was a driver that invoked the gcc driver once per `-arch` or 
some such. One of the early goals of clang was to just have a driver, but no 
driver driver.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D137338/new/

https://reviews.llvm.org/D137338

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D137338: Fix dupe word typos

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

I landed the Clang bits in 94738a5ac34283bb034b022602b9f9e93d67081f 
, thank 
you for the cleanup!


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D137338/new/

https://reviews.llvm.org/D137338

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D137338: Fix dupe word typos

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added a comment.

LGTM for changes under `clang-tools-extra/clangd`


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D137338/new/

https://reviews.llvm.org/D137338

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D137338: Fix dupe word typos

[Tom leet via Phabricator via cfe-commits]

Rageking8 added a comment.

In D137338#3912135 , @aaron.ballman 
wrote:

> In D137338#3907281 , @Rageking8 
> wrote:
>
>> I am ok with you guys taking the parts of this revision that you reviewed 
>> and directly commiting them to the repo, just like how the person above did 
>> it. This is to break the revision up to smaller parts.
>
> What name and email address would you like us to use for patch attribution 
> when landing these bits?

Rageking8
tomlee...@gmail.com


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D137338/new/

https://reviews.llvm.org/D137338

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D137338: Fix dupe word typos

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D137338#3907281 , @Rageking8 wrote:

> I am ok with you guys taking the parts of this revision that you reviewed and 
> directly commiting them to the repo, just like how the person above did it. 
> This is to break the revision up to smaller parts.

What name and email address would you like us to use for patch attribution when 
landing these bits?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D137338/new/

https://reviews.llvm.org/D137338

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D137338: Fix dupe word typos

[Tom leet via Phabricator via cfe-commits]

Rageking8 added a comment.

I am ok with you guys taking the parts of this revision that you reviewed and 
directly commiting them to the repo, just like how the person above did it. 
This is to break the revision up to smaller parts.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D137338/new/

https://reviews.llvm.org/D137338

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D137338: Fix dupe word typos

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: clang/lib/CodeGen/CodeGenTBAA.cpp:341
 if (const CXXRecordDecl *CXXRD = dyn_cast(RD)) {
-  // Handle C++ base classes. Non-virtual bases can treated a a kind of
+  // Handle C++ base classes. Non-virtual bases can treated a kind of
   // field. Virtual bases are more complex and omitted, but avoid an

curdeius wrote:
> aaron.ballman wrote:
> > I think this should read:
> > ```
> >  // Handle C++ base classes. Non-virtual bases can treated as a kind of
> > ```
> A mistake: can *be* treated.
Oh gosh, good catch! 


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D137338/new/

https://reviews.llvm.org/D137338

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D137338: Fix dupe word typos

[Marek Kurdej via Phabricator via cfe-commits]

curdeius added inline comments.



Comment at: clang/lib/CodeGen/CodeGenTBAA.cpp:341
 if (const CXXRecordDecl *CXXRD = dyn_cast(RD)) {
-  // Handle C++ base classes. Non-virtual bases can treated a a kind of
+  // Handle C++ base classes. Non-virtual bases can treated a kind of
   // field. Virtual bases are more complex and omitted, but avoid an

aaron.ballman wrote:
> I think this should read:
> ```
>  // Handle C++ base classes. Non-virtual bases can treated as a kind of
> ```
A mistake: can *be* treated.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D137338/new/

https://reviews.llvm.org/D137338

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D137338: Fix dupe word typos

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

The clang parts generally LG, but I spotted a few things. Thank you for this 
cleanup effort!




Comment at: clang/include/clang/ASTMatchers/ASTMatchersInternal.h:467
 
-  /// Return a matcher that that points to the same implementation, but sets 
the
+  /// Return a matcher that points to the same implementation, but sets the
   ///   traversal kind.

This is a good change, but you also need to run 
`clang/docs/tools/dump_ast_matchers.py` to regenerate the public-facing 
documentation that is generated from this file.



Comment at: clang/lib/CodeGen/CodeGenTBAA.cpp:341
 if (const CXXRecordDecl *CXXRD = dyn_cast(RD)) {
-  // Handle C++ base classes. Non-virtual bases can treated a a kind of
+  // Handle C++ base classes. Non-virtual bases can treated a kind of
   // field. Virtual bases are more complex and omitted, but avoid an

I think this should read:
```
 // Handle C++ base classes. Non-virtual bases can treated as a kind of
```



Comment at: clang/lib/Driver/ToolChains/Darwin.cpp:47
   // complete architecture list, nor a reasonable subset. The problem is that
-  // historically the driver driver accepts this and also ties its -march=
   // handling to the architecture name, so we need to be careful before 
removing

This is duplicated just often enough and in just enough contexts where I think 
"driver driver" means "the driver that drives another driver", but it'd be nice 
if someone else could confirm or deny that.



Comment at: clang/lib/Driver/ToolChains/Gnu.cpp:87
 
-  // If using a driver driver, force the arch.
   if (getToolChain().getTriple().isOSDarwin()) {

Same for this one as above.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D137338/new/

https://reviews.llvm.org/D137338

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D137338: Fix dupe word typos

[Jez Ng via Phabricator via cfe-commits]

int3 added a comment.

lld/MachO changes lgtm




Comment at: lld/MachO/Driver.cpp:1386
 // ld64 does something really weird. It attempts to realign the value to 
the
-// page size, but assumes the the page size is 4K. This doesn't work with
+// page size, but assumes the page size is 4K. This doesn't work with
 // most of Apple's ARM64 devices, which use a page size of 16K. This means

"assumes that the page size" sounds more natural and is probably what was 
intended



Comment at: lld/MachO/UnwindInfoSection.cpp:576-577
 // entries by using the regular format. This can happen when there
-// are many unique encodings, and we we saturated the local
+// are many unique encodings, and we saturated the local
 // encoding table early.
 if (i < cuIndices.size() &&

I think we can reflow this paragraph to max the line length


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D137338/new/

https://reviews.llvm.org/D137338

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D137338: Fix dupe word typos

[Amir Ayupov via Phabricator via cfe-commits]

Amir accepted this revision.
Amir added a comment.

BOLT changes LGTM


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D137338/new/

https://reviews.llvm.org/D137338

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D137338: Fix dupe word typos

[Siva Chandra via Phabricator via cfe-commits]

sivachandra accepted this revision.
sivachandra added a comment.

Changes in the libc directory LGTM.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D137338/new/

https://reviews.llvm.org/D137338

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D137338: Fix dupe word typos

[Shilei Tian via Phabricator via cfe-commits]

tianshilei1992 added a comment.

Changes to `openmp` look good to me.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D137338/new/

https://reviews.llvm.org/D137338

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D137338: Fix dupe word typos

[David Spickett via Phabricator via cfe-commits]

DavidSpickett added a comment.

Looked at the lldb changes, some comments for you. If you want to get a "looks 
good" for those please submit a separate review with only the lldb parts and 
I'll review that instead.

As others said, appreciate the effort but the review process doesn't scale well 
to so many changes in one patch.




Comment at: lldb/include/lldb/Target/Process.h:1204
   /// platform that might itself be running natively, but have different
-  /// heuristics for figuring out which OS is is emulating.
+  /// heuristics for figuring out which OS is emulating.
   ///

This should be "which OS it is emulating".



Comment at: lldb/source/Symbol/LineTable.cpp:92
 // after the prologue.
-// Instead of it it is issuing a line table entry for the first instruction
+// Instead of it is issuing a line table entry for the first instruction
 // of the prologue and one for the first instruction after the prologue. If

This should be "Instead it is issuing".



Comment at: lldb/source/Target/RegisterContextUnwind.cpp:701
   // can have arbitrary number of frames with the same CFA, but more then 2 is
-  // very very unlikely)
 

This one is intended. It's not very scientific but hey, it's getting the point 
across.



Comment at: lldb/tools/lldb-vscode/JSONUtils.cpp:1045
   auto type_cstr = type_obj.GetDisplayTypeName();
-  // If we have a type with many many children, we would like to be able to
   // give a hint to the IDE that the type has indexed children so that the

This one is intended. Given the context of batching work, it's making the point 
for very large amounts of children.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D137338/new/

https://reviews.llvm.org/D137338

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D137338: Fix dupe word typos

[Louis Dionne via Phabricator via cfe-commits]

ldionne accepted this revision.
ldionne added a comment.

Thanks for the fixes. LGTM for `libcxx/`, `libcxxabi/` and `libunwind/`.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D137338/new/

https://reviews.llvm.org/D137338

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D137338: Fix dupe word typos

[Aart Bik via Phabricator via cfe-commits]

aartbik added a comment.

LGTM for sparse changes

(note that you could have made your life a bit easier if you had broken this 
revision up at least over different projects, getting a global "LGTM" from 
somebody may be a bit hard now ;-)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D137338/new/

https://reviews.llvm.org/D137338

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D137338: Fix dupe word typos

[Michael Kruse via Phabricator via cfe-commits]

Meinersbur added a comment.

Changes in `/polly/` look good to me.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D137338/new/

https://reviews.llvm.org/D137338

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D137338: Fix dupe word typos

[Jay Foad via Phabricator via cfe-commits]

foad added a comment.

I committed the lib/Target/AMDGPU parts as 5073ae2a883f 
.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D137338/new/

https://reviews.llvm.org/D137338

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D137338: Fix dupe word typos

[Alexander Potapenko via Phabricator via cfe-commits]

glider added subscribers: pcc, glider.
glider added inline comments.



Comment at: clang/docs/DataFlowSanitizerDesign.rst:54
 
-  /// Returns whether the given label label contains the label elem.
+  /// Returns whether the given label contains the label elem.
   int dfsan_has_label(dfsan_label label, dfsan_label elem);

I believe the word duplication is on purpose here. I'd rewrite the comment as:

```
Returns whether the given label @label contains the label @elem.
```

@pcc, what do you think?



Comment at: compiler-rt/include/sanitizer/dfsan_interface.h:65
 
-/// Returns whether the given label label contains the label elem.
+/// Returns whether the given label contains the label elem.
 int dfsan_has_label(dfsan_label label, dfsan_label elem);

Same as clang/docs/DataFlowSanitizerDesign.rst above.



Comment at: compiler-rt/lib/asan/asan_allocator.cpp:1161
   // the `pthread_create()` interceptor doesn't wait for the child thread to
-  // start before returning and thus loosing the the only live reference to the
+  // start before returning and thus loosing the only live reference to the
   // heap object on the stack.

Ack.



Comment at: 
compiler-rt/lib/sanitizer_common/tests/sanitizer_stacktrace_test.cpp:211
 
-  // This the the truncation case.
+  // This the truncation case.
   ASSERT_GT(actual_len, sizeof(tinybuf));

Ack.



Comment at: compiler-rt/lib/tsan/rtl-old/tsan_interceptors_posix.cpp:2510
 // Can't use internal_memcpy, because it copies byte-by-byte,
-// and signal handler reads the handler concurrently. It it can read
+// and signal handler reads the handler concurrently. It can read
 // some bytes from old value and some bytes from new value.

Ack.



Comment at: compiler-rt/lib/tsan/rtl/tsan_interceptors_posix.cpp:2529
 // Can't use internal_memcpy, because it copies byte-by-byte,
-// and signal handler reads the handler concurrently. It it can read
+// and signal handler reads the handler concurrently. It can read
 // some bytes from old value and some bytes from new value.

Ack.



Comment at: llvm/lib/Transforms/Instrumentation/DataFlowSanitizer.cpp:540
   Type *getShadowTy(Type *OrigTy);
-  /// Returns the shadow type of of V's type.
+  /// Returns the shadow type of V's type.
   Type *getShadowTy(Value *V);

Ack.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D137338/new/

https://reviews.llvm.org/D137338

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D137338: Fix dupe word typos

[James Henderson via Phabricator via cfe-commits]

jhenderson added a comment.
Herald added a reviewer: jdoerfert.
Herald added subscribers: Michael137, sstefan1, JDevlieghere.

This is going to be impossible to cleanly review as-is. Could it be broken into 
lots of smaller chunks?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D137338/new/

https://reviews.llvm.org/D137338

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits