[PATCH] D156636: [InstCombine] Deduce `align` and `nonnull` return attributes for `llvm.ptrmask`

[Noah Goldstein via Phabricator via cfe-commits]

goldstein.w.n abandoned this revision.
goldstein.w.n added a comment.

This has been ported to github PR.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D156636

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

[PATCH] D157485: [X86][RFC] Support new feature AVX10

[Noah Goldstein via Phabricator via cfe-commits]

goldstein.w.n added inline comments.



Comment at: clang/lib/Basic/Targets/X86.h:99
+  bool HasAVX10_1 = false;
+  bool HasAVX10_512BIT = false;
   bool HasAVX512CD = false;

Maybe should be HasAVX10_1_512? As brought up the rfc, there might be an 
avx10.2-512

Likewise elsewhere, or is this to match GCC?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157485

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

[PATCH] D156556: [AggressiveInstCombine][NFC] Fix typo

[Noah Goldstein via Phabricator via cfe-commits]

goldstein.w.n accepted this revision.
goldstein.w.n added a comment.
This revision is now accepted and ready to land.

In D156556#4555036 , @kitaisreal 
wrote:

> In D156556#4554436 , @goldstein.w.n 
> wrote:
>
>> Can you mark the title as NFC?
>
> It seems that title of revision is "[AggressiveInstCombine][NFC] Fix typo". 
> Do you mean to put NFC at the beginning ?

No its good. I just can't read.
LGTM.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D156556

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

[PATCH] D156556: [AggressiveInstCombine][NFC] Fix typo

[Noah Goldstein via Phabricator via cfe-commits]

goldstein.w.n added a comment.

Can you mark the title as NFC?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D156556

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

[PATCH] D156636: [InstCombine] Deduce `align` and `nonnull` return attributes for `llvm.ptrmask`

[Noah Goldstein via Phabricator via cfe-commits]

goldstein.w.n created this revision.
goldstein.w.n added reviewers: nikic, RKSimon.
Herald added subscribers: StephenFan, hiraditya.
Herald added a project: All.
goldstein.w.n requested review of this revision.
Herald added projects: clang, LLVM.
Herald added subscribers: llvm-commits, cfe-commits.

We can deduce the former based on the mask / incoming pointer
alignment.  We can set the latter based if know the result in non-zero
(this is essentially just caching our analysis result).


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D156636

Files:
  clang/test/CodeGen/arm64_32-vaarg.c
  llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp
  llvm/test/Transforms/InstCombine/align-addr.ll
  llvm/test/Transforms/InstCombine/consecutive-ptrmask.ll
  llvm/test/Transforms/InstCombine/ptrmask.ll

Index: llvm/test/Transforms/InstCombine/ptrmask.ll
===
--- llvm/test/Transforms/InstCombine/ptrmask.ll
+++ llvm/test/Transforms/InstCombine/ptrmask.ll
@@ -8,7 +8,7 @@
 ; CHECK-LABEL: define ptr @ptrmask_combine_consecutive_preserve_attrs
 ; CHECK-SAME: (ptr [[P0:%.*]], i64 [[M1:%.*]]) {
 ; CHECK-NEXT:[[TMP1:%.*]] = and i64 [[M1]], 224
-; CHECK-NEXT:[[R:%.*]] = call ptr @llvm.ptrmask.p0.i64(ptr [[P0]], i64 [[TMP1]])
+; CHECK-NEXT:[[R:%.*]] = call align 32 ptr @llvm.ptrmask.p0.i64(ptr [[P0]], i64 [[TMP1]])
 ; CHECK-NEXT:ret ptr [[R]]
 ;
   %pm0 = call ptr @llvm.ptrmask.p0.i64(ptr %p0, i64 224)
@@ -31,7 +31,7 @@
 define ptr @ptrmask_combine_consecutive_preserve_attrs_todo0(ptr %p0) {
 ; CHECK-LABEL: define ptr @ptrmask_combine_consecutive_preserve_attrs_todo0
 ; CHECK-SAME: (ptr [[P0:%.*]]) {
-; CHECK-NEXT:[[PM0:%.*]] = call noalias ptr @llvm.ptrmask.p0.i64(ptr [[P0]], i64 224)
+; CHECK-NEXT:[[PM0:%.*]] = call noalias align 32 ptr @llvm.ptrmask.p0.i64(ptr [[P0]], i64 224)
 ; CHECK-NEXT:ret ptr [[PM0]]
 ;
   %pm0 = call noalias ptr @llvm.ptrmask.p0.i64(ptr %p0, i64 224)
@@ -42,7 +42,7 @@
 define ptr @ptrmask_combine_consecutive_preserve_attrs_todo1(ptr %p0) {
 ; CHECK-LABEL: define ptr @ptrmask_combine_consecutive_preserve_attrs_todo1
 ; CHECK-SAME: (ptr [[P0:%.*]]) {
-; CHECK-NEXT:[[PM0:%.*]] = call ptr @llvm.ptrmask.p0.i64(ptr [[P0]], i64 224)
+; CHECK-NEXT:[[PM0:%.*]] = call align 32 ptr @llvm.ptrmask.p0.i64(ptr [[P0]], i64 224)
 ; CHECK-NEXT:ret ptr [[PM0]]
 ;
   %pm0 = call ptr @llvm.ptrmask.p0.i64(ptr %p0, i64 224)
@@ -53,7 +53,7 @@
 define ptr @ptrmask_combine_consecutive_preserve_attrs_todo2(ptr %p0) {
 ; CHECK-LABEL: define ptr @ptrmask_combine_consecutive_preserve_attrs_todo2
 ; CHECK-SAME: (ptr [[P0:%.*]]) {
-; CHECK-NEXT:[[PM0:%.*]] = call noalias ptr @llvm.ptrmask.p0.i32(ptr [[P0]], i32 224)
+; CHECK-NEXT:[[PM0:%.*]] = call noalias align 32 ptr @llvm.ptrmask.p0.i32(ptr [[P0]], i32 224)
 ; CHECK-NEXT:ret ptr [[PM0]]
 ;
   %pm0 = call noalias ptr @llvm.ptrmask.p0.i32(ptr %p0, i32 224)
@@ -64,9 +64,9 @@
 define ptr @ptrmask_combine_add_nonnull(ptr %p) {
 ; CHECK-LABEL: define ptr @ptrmask_combine_add_nonnull
 ; CHECK-SAME: (ptr [[P:%.*]]) {
-; CHECK-NEXT:[[PM0:%.*]] = call ptr @llvm.ptrmask.p0.i64(ptr [[P]], i64 -64)
+; CHECK-NEXT:[[PM0:%.*]] = call align 64 ptr @llvm.ptrmask.p0.i64(ptr [[P]], i64 -64)
 ; CHECK-NEXT:[[PGEP:%.*]] = getelementptr i8, ptr [[PM0]], i64 33
-; CHECK-NEXT:[[R:%.*]] = call ptr @llvm.ptrmask.p0.i64(ptr [[PGEP]], i64 -16)
+; CHECK-NEXT:[[R:%.*]] = call nonnull align 32 ptr @llvm.ptrmask.p0.i64(ptr [[PGEP]], i64 -16)
 ; CHECK-NEXT:ret ptr [[R]]
 ;
   %pm0 = call ptr @llvm.ptrmask.p0.i64(ptr %p, i64 -64)
@@ -78,7 +78,7 @@
 define ptr @ptrmask_combine_add_alignment(ptr %p) {
 ; CHECK-LABEL: define ptr @ptrmask_combine_add_alignment
 ; CHECK-SAME: (ptr [[P:%.*]]) {
-; CHECK-NEXT:[[R:%.*]] = call ptr @llvm.ptrmask.p0.i64(ptr [[P]], i64 -64)
+; CHECK-NEXT:[[R:%.*]] = call align 64 ptr @llvm.ptrmask.p0.i64(ptr [[P]], i64 -64)
 ; CHECK-NEXT:ret ptr [[R]]
 ;
   %r = call ptr @llvm.ptrmask.p0.i64(ptr %p, i64 -64)
@@ -88,7 +88,7 @@
 define ptr @ptrmask_combine_add_alignment2(ptr align 32 %p) {
 ; CHECK-LABEL: define ptr @ptrmask_combine_add_alignment2
 ; CHECK-SAME: (ptr align 32 [[P:%.*]]) {
-; CHECK-NEXT:[[R:%.*]] = call ptr @llvm.ptrmask.p0.i32(ptr [[P]], i32 -64)
+; CHECK-NEXT:[[R:%.*]] = call align 64 ptr @llvm.ptrmask.p0.i32(ptr [[P]], i32 -64)
 ; CHECK-NEXT:ret ptr [[R]]
 ;
   %r = call ptr @llvm.ptrmask.p0.i32(ptr %p, i32 -64)
@@ -98,7 +98,7 @@
 define ptr @ptrmask_combine_improve_alignment(ptr %p) {
 ; CHECK-LABEL: define ptr @ptrmask_combine_improve_alignment
 ; CHECK-SAME: (ptr [[P:%.*]]) {
-; CHECK-NEXT:[[R:%.*]] = call align 32 ptr @llvm.ptrmask.p0.i32(ptr [[P]], i32 -64)
+; CHECK-NEXT:[[R:%.*]] = call align 64 ptr @llvm.ptrmask.p0.i32(ptr [[P]], i32 -64)
 ; CHECK-NEXT:ret ptr [[R]]
 ;
   %r = call align 32 ptr @llvm.ptrmask.p0.i32(ptr %p, i32 -64)
Index: llvm/test/Transforms/InstCombine/consecutive

[PATCH] D152226: [FunctionAttrs] Propagate some func/arg/ret attributes from caller to callsite (WIP)

[Noah Goldstein via Phabricator via cfe-commits]

goldstein.w.n added inline comments.



Comment at: llvm/lib/Transforms/Utils/InferCallsiteAttrs.cpp:597
+  // callsite violating the constraint.
+  if (checkCallerDoesNotAccessMemory() && !CB->doesNotAccessMemory()) {
+// Wait until we know we actually need it to do potentially expensive

goldstein.w.n wrote:
> nikic wrote:
> > For these you generally want to query `getMemoryEffects()` on the 
> > caller/callee once and then work on that representation, instead of doing 
> > separate queries for everything. This may allow more precise handling and 
> > likely reduces the compile-time impact, as doing these attribute lookups is 
> > quite expensive.
> Re-running compile time checks, will post revised numbers tomorrow.
No real impact on compile time.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152226

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

[PATCH] D152226: [FunctionAttrs] Propagate some func/arg/ret attributes from caller to callsite (WIP)

[Noah Goldstein via Phabricator via cfe-commits]

goldstein.w.n added inline comments.



Comment at: llvm/lib/Transforms/IPO/FunctionAttrs.cpp:1764
+  if (ICA.processFunction(F))
+Changed.insert(F);
+  }

Do we actually need to add the function to the changed list here? We aren't 
modifying the functions attributes, only callsites within the function.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152226

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

[PATCH] D152226: [FunctionAttrs] Propagate some func/arg/ret attributes from caller to callsite (WIP)

[Noah Goldstein via Phabricator via cfe-commits]

goldstein.w.n added inline comments.



Comment at: llvm/lib/Transforms/Utils/InferCallsiteAttrs.cpp:597
+  // callsite violating the constraint.
+  if (checkCallerDoesNotAccessMemory() && !CB->doesNotAccessMemory()) {
+// Wait until we know we actually need it to do potentially expensive

nikic wrote:
> For these you generally want to query `getMemoryEffects()` on the 
> caller/callee once and then work on that representation, instead of doing 
> separate queries for everything. This may allow more precise handling and 
> likely reduces the compile-time impact, as doing these attribute lookups is 
> quite expensive.
Re-running compile time checks, will post revised numbers tomorrow.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152226

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

[PATCH] D152226: [FunctionAttrs] Propagate some func/arg/ret attributes from caller to callsite (WIP)

[Noah Goldstein via Phabricator via cfe-commits]

goldstein.w.n updated this revision to Diff 531233.
goldstein.w.n added a comment.

Remove unused check count. Use memory effects directly instead of
function/callbase attribute API


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152226

Files:
  clang/test/CodeGen/LoongArch/inline-asm-constraints.c
  clang/test/CodeGen/LoongArch/inline-asm-operand-modifiers.c
  clang/test/CodeGen/LoongArch/intrinsic-la32.c
  clang/test/CodeGen/LoongArch/intrinsic-la64.c
  clang/test/CodeGen/PowerPC/builtins-ppc-build-pair-mma.c
  clang/test/CodeGen/PowerPC/builtins-ppc-pair-mma.c
  
clang/test/CodeGen/PowerPC/builtins-ppc-xlcompat-LoadReseve-StoreCond-64bit-only.c
  clang/test/CodeGen/PowerPC/builtins-ppc-xlcompat-LoadReseve-StoreCond.c
  clang/test/CodeGen/PowerPC/ppc64-inline-asm.c
  clang/test/CodeGen/RISCV/riscv-inline-asm.c
  clang/test/CodeGen/RISCV/rvv-intrinsics-handcrafted/vwrite-csr.c
  clang/test/CodeGen/X86/fma-builtins-constrained.c
  clang/test/CodeGen/X86/ms-x86-intrinsics.c
  clang/test/CodeGen/aarch64-bf16-ldst-intrinsics.c
  clang/test/CodeGen/aarch64-ls64-inline-asm.c
  clang/test/CodeGen/aarch64-sve2-intrinsics/acle_sve2_ldnt1.c
  clang/test/CodeGen/aarch64-sve2-intrinsics/acle_sve2_ldnt1sb.c
  clang/test/CodeGen/aarch64-sve2-intrinsics/acle_sve2_ldnt1sh.c
  clang/test/CodeGen/aarch64-sve2-intrinsics/acle_sve2_ldnt1sw.c
  clang/test/CodeGen/aarch64-sve2-intrinsics/acle_sve2_ldnt1ub.c
  clang/test/CodeGen/aarch64-sve2-intrinsics/acle_sve2_ldnt1uh.c
  clang/test/CodeGen/aarch64-sve2-intrinsics/acle_sve2_ldnt1uw.c
  clang/test/CodeGen/aarch64-sve2-intrinsics/acle_sve2_stnt1.c
  clang/test/CodeGen/aarch64-sve2-intrinsics/acle_sve2_stnt1b.c
  clang/test/CodeGen/aarch64-sve2-intrinsics/acle_sve2_stnt1h.c
  clang/test/CodeGen/aarch64-sve2-intrinsics/acle_sve2_stnt1w.c
  clang/test/CodeGen/aarch64-sve2-intrinsics/acle_sve2_whilege.c
  clang/test/CodeGen/aarch64-sve2-intrinsics/acle_sve2_whilegt.c
  clang/test/CodeGen/aarch64-sve2-intrinsics/acle_sve2_whilerw-bfloat.c
  clang/test/CodeGen/aarch64-sve2-intrinsics/acle_sve2_whilerw.c
  clang/test/CodeGen/aarch64-sve2-intrinsics/acle_sve2_whilewr-bfloat.c
  clang/test/CodeGen/aarch64-sve2-intrinsics/acle_sve2_whilewr.c
  
clang/test/CodeGen/aarch64_neon_sve_bridge_intrinsics/acle_neon_sve_bridge_dup_neonq.c
  
clang/test/CodeGen/aarch64_neon_sve_bridge_intrinsics/acle_neon_sve_bridge_get_neonq.c
  
clang/test/CodeGen/aarch64_neon_sve_bridge_intrinsics/acle_neon_sve_bridge_set_neonq.c
  clang/test/CodeGen/attr-arm-sve-vector-bits-cast.c
  clang/test/CodeGen/attr-riscv-rvv-vector-bits-call.c
  clang/test/CodeGen/attr-riscv-rvv-vector-bits-cast.c
  clang/test/CodeGen/msp430-builtins.c
  clang/test/CodeGen/nofpclass.c
  clang/test/Headers/wasm.c
  llvm/include/llvm/Transforms/Utils/InferCallsiteAttrs.h
  llvm/lib/Transforms/IPO/FunctionAttrs.cpp
  llvm/lib/Transforms/Utils/CMakeLists.txt
  llvm/lib/Transforms/Utils/InferCallsiteAttrs.cpp
  llvm/test/Other/cgscc-devirt-iteration.ll
  llvm/test/Transforms/FunctionAttrs/nonnull.ll
  llvm/test/Transforms/FunctionAttrs/readattrs.ll
  llvm/test/Transforms/FunctionAttrs/willreturn-callsites.ll
  llvm/test/Transforms/MergeFunc/mergefunc-preserve-debug-info.ll
  llvm/test/Transforms/PhaseOrdering/X86/loop-idiom-vs-indvars.ll
  llvm/test/Transforms/PhaseOrdering/memset-tail.ll

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

[PATCH] D152226: [FunctionAttrs] Propagate some func/arg/ret attributes from caller to callsite (WIP)

[Noah Goldstein via Phabricator via cfe-commits]

goldstein.w.n added a comment.

In D152226#4418978 , @goldstein.w.n 
wrote:

> In D152226#4418498 , @jdoerfert 
> wrote:
>
>> FWIW, I wish the time that went into this would have been to improve and 
>> enable the (lightweight) Attributor pass.
>> Anyway, I'm still doing that, and the latest numbers didn't look that bad. I 
>> will start to upstream stuff that I found.
>> At this point it might be good to understand if the (lightweight) Attributor 
>> would solve your problems or what would be left.
>
> With the way the `InferCallsiteAttrs` is written, it should be equally usable 
> in attributor as it is in FunctionAttrs. Guess my
> feeling was want to get this in s.t its actually usable. Once LW attributor 
> becomes usable should be easy to apply `InferCallsiteAttrs`
> there.

Guess I'm assuming this will be usable in the Attributor. That is the goal. If 
its not the case am happy to refactor so that it is.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152226

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

[PATCH] D152226: [FunctionAttrs] Propagate some func/arg/ret attributes from caller to callsite (WIP)

[Noah Goldstein via Phabricator via cfe-commits]

goldstein.w.n added a comment.

In D152226#4418498 , @jdoerfert wrote:

> FWIW, I wish the time that went into this would have been to improve and 
> enable the (lightweight) Attributor pass.
> Anyway, I'm still doing that, and the latest numbers didn't look that bad. I 
> will start to upstream stuff that I found.
> At this point it might be good to understand if the (lightweight) Attributor 
> would solve your problems or what would be left.

With the way the `InferCallsiteAttrs` is written, it should be equally usable 
in attributor as it is in FunctionAttrs. Guess my
feeling was want to get this in s.t its actually usable. Once LW attributor 
becomes usable should be easy to apply `InferCallsiteAttrs`
there.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152226

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

[PATCH] D152226: [FunctionAttrs] Propagate some func/arg/ret attributes from caller to callsite (WIP)

[Noah Goldstein via Phabricator via cfe-commits]

goldstein.w.n updated this revision to Diff 530798.
goldstein.w.n added a comment.

fixes


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152226

Files:
  clang/test/CodeGen/LoongArch/inline-asm-constraints.c
  clang/test/CodeGen/LoongArch/inline-asm-operand-modifiers.c
  clang/test/CodeGen/LoongArch/intrinsic-la32.c
  clang/test/CodeGen/LoongArch/intrinsic-la64.c
  clang/test/CodeGen/PowerPC/builtins-ppc-build-pair-mma.c
  clang/test/CodeGen/PowerPC/builtins-ppc-pair-mma.c
  
clang/test/CodeGen/PowerPC/builtins-ppc-xlcompat-LoadReseve-StoreCond-64bit-only.c
  clang/test/CodeGen/PowerPC/builtins-ppc-xlcompat-LoadReseve-StoreCond.c
  clang/test/CodeGen/PowerPC/ppc64-inline-asm.c
  clang/test/CodeGen/RISCV/riscv-inline-asm.c
  clang/test/CodeGen/RISCV/rvv-intrinsics-handcrafted/vwrite-csr.c
  clang/test/CodeGen/X86/fma-builtins-constrained.c
  clang/test/CodeGen/X86/ms-x86-intrinsics.c
  clang/test/CodeGen/aarch64-bf16-ldst-intrinsics.c
  clang/test/CodeGen/aarch64-ls64-inline-asm.c
  clang/test/CodeGen/aarch64-sve2-intrinsics/acle_sve2_ldnt1.c
  clang/test/CodeGen/aarch64-sve2-intrinsics/acle_sve2_ldnt1sb.c
  clang/test/CodeGen/aarch64-sve2-intrinsics/acle_sve2_ldnt1sh.c
  clang/test/CodeGen/aarch64-sve2-intrinsics/acle_sve2_ldnt1sw.c
  clang/test/CodeGen/aarch64-sve2-intrinsics/acle_sve2_ldnt1ub.c
  clang/test/CodeGen/aarch64-sve2-intrinsics/acle_sve2_ldnt1uh.c
  clang/test/CodeGen/aarch64-sve2-intrinsics/acle_sve2_ldnt1uw.c
  clang/test/CodeGen/aarch64-sve2-intrinsics/acle_sve2_stnt1.c
  clang/test/CodeGen/aarch64-sve2-intrinsics/acle_sve2_stnt1b.c
  clang/test/CodeGen/aarch64-sve2-intrinsics/acle_sve2_stnt1h.c
  clang/test/CodeGen/aarch64-sve2-intrinsics/acle_sve2_stnt1w.c
  clang/test/CodeGen/aarch64-sve2-intrinsics/acle_sve2_whilege.c
  clang/test/CodeGen/aarch64-sve2-intrinsics/acle_sve2_whilegt.c
  clang/test/CodeGen/aarch64-sve2-intrinsics/acle_sve2_whilerw-bfloat.c
  clang/test/CodeGen/aarch64-sve2-intrinsics/acle_sve2_whilerw.c
  clang/test/CodeGen/aarch64-sve2-intrinsics/acle_sve2_whilewr-bfloat.c
  clang/test/CodeGen/aarch64-sve2-intrinsics/acle_sve2_whilewr.c
  
clang/test/CodeGen/aarch64_neon_sve_bridge_intrinsics/acle_neon_sve_bridge_dup_neonq.c
  
clang/test/CodeGen/aarch64_neon_sve_bridge_intrinsics/acle_neon_sve_bridge_get_neonq.c
  
clang/test/CodeGen/aarch64_neon_sve_bridge_intrinsics/acle_neon_sve_bridge_set_neonq.c
  clang/test/CodeGen/attr-arm-sve-vector-bits-cast.c
  clang/test/CodeGen/attr-riscv-rvv-vector-bits-call.c
  clang/test/CodeGen/attr-riscv-rvv-vector-bits-cast.c
  clang/test/CodeGen/msp430-builtins.c
  clang/test/CodeGen/nofpclass.c
  clang/test/Headers/wasm.c
  llvm/include/llvm/Transforms/Utils/InferCallsiteAttrs.h
  llvm/lib/Transforms/IPO/FunctionAttrs.cpp
  llvm/lib/Transforms/Utils/CMakeLists.txt
  llvm/lib/Transforms/Utils/InferCallsiteAttrs.cpp
  llvm/test/Other/cgscc-devirt-iteration.ll
  llvm/test/Transforms/FunctionAttrs/nonnull.ll
  llvm/test/Transforms/FunctionAttrs/readattrs.ll
  llvm/test/Transforms/FunctionAttrs/willreturn-callsites.ll
  llvm/test/Transforms/MergeFunc/mergefunc-preserve-debug-info.ll
  llvm/test/Transforms/PhaseOrdering/X86/loop-idiom-vs-indvars.ll
  llvm/test/Transforms/PhaseOrdering/memset-tail.ll

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

[PATCH] D152226: [FunctionAttrs] Propagate some func/arg/ret attributes from caller to callsite (WIP)

[Noah Goldstein via Phabricator via cfe-commits]

goldstein.w.n marked 8 inline comments as done.
goldstein.w.n added inline comments.



Comment at: llvm/include/llvm/Transforms/Utils/InferCallsiteAttrs.h:28
+  // relatively high value is okay.
+  static constexpr unsigned kMaxChecks = UINT_MAX;
+

arsenm wrote:
> Does changing this to something meaningful change the compile time results?
lowered to 100, doesn't seem to help:
https://llvm-compile-time-tracker.com/compare.php?from=79feb6e78b818e33ec69abdc58c5f713d691554f&to=79e0b7e6f0547de9ad534c02869437160028af01&stat=instructions:u

Lowered to 10, doesn't seem to help either:
https://llvm-compile-time-tracker.com/compare.php?from=79feb6e78b818e33ec69abdc58c5f713d691554f&to=9e3f35bc8b6f0f261698ba1ce18229d1b8d9cdb6&stat=instructions%3Au

I'll drop unless you think there is a reason to keep it around.



Comment at: llvm/include/llvm/Transforms/Utils/InferCallsiteAttrs.h:63
+  bool checkCallerHasFnAttr(Attribute::AttrKind Attr) const {
+return (CxtCB && CxtCB->hasFnAttr(Attr)) || Caller->hasFnAttribute(Attr);
+  };

arsenm wrote:
> Without looking at the rest of the context I would assume CxtB is always 
> valid if the class is valid and all the null checks should be dropped
Not quite, so `CxtB` is used to specify propagation of function attributes from 
a specific callsite. This may be useful in inline. i.e if we have:

foo -> bar -> baz
and
fiz -> bar -> buz

If `bar` in `foo` is getting inlined, we can propagate not only the function 
attributes of `bar`, but also the callsite attributes of that specific call to 
`bar` to any callsites (so `baz`).

At the moment its always null in fact.



Comment at: llvm/lib/Transforms/Utils/InferCallsiteAttrs.cpp:343
+  // have derived from an argument. Finally, allocas/leaked mallocs in general
+  // are difficult (so we avoid them entirely). Callsites can arbitrarily
+  // store pointers in allocas for use later without violating a nocapture

arsenm wrote:
> 
I think callsite is regularly refererred to as singular word (in other files 
too) so prefer to keep as is. That okay?



Comment at: llvm/lib/Transforms/Utils/InferCallsiteAttrs.cpp:357
+  // the caller will apply to the callsites throw. If the caller has a landing
+  // padd, its possible for the callsite to capture a pointer in a throw that
+  // is later cleared by the caller.

arsenm wrote:
> 
did 'pad' -> 'padd' but prefer to keep callsite as one word



Comment at: llvm/lib/Transforms/Utils/InferCallsiteAttrs.cpp:692
+  else
+memset(&CurFnInfo, kMaybe, sizeof(CurFnInfo));
+  Caller = ParentFunc;

arsenm wrote:
> Not very C++y, use the default constructor?
If `PreserveCache` is true, then we can redo same function (non-sequentially) 
and get saved analysis. If you think this is meaningless and we will always do 
one-off functions I can change to just construct with a caller (although I 
think for inlining this might come up).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152226

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

[PATCH] D152226: [FunctionAttrs] Propagate some func/arg/ret attributes from caller to callsite (WIP)

[Noah Goldstein via Phabricator via cfe-commits]

This revision was not accepted when it landed; it landed in state "Needs 
Review".
This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
goldstein.w.n marked an inline comment as done.
Closed by commit rG4fa971ff62c3: [FunctionAttrs] Propagate some func/arg/ret 
attributes from caller to callsite… (authored by goldstein.w.n).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152226

Files:
  clang/test/CodeGen/LoongArch/inline-asm-constraints.c
  clang/test/CodeGen/LoongArch/inline-asm-operand-modifiers.c
  clang/test/CodeGen/LoongArch/intrinsic-la32.c
  clang/test/CodeGen/LoongArch/intrinsic-la64.c
  clang/test/CodeGen/PowerPC/builtins-ppc-build-pair-mma.c
  clang/test/CodeGen/PowerPC/builtins-ppc-pair-mma.c
  
clang/test/CodeGen/PowerPC/builtins-ppc-xlcompat-LoadReseve-StoreCond-64bit-only.c
  clang/test/CodeGen/PowerPC/builtins-ppc-xlcompat-LoadReseve-StoreCond.c
  clang/test/CodeGen/PowerPC/ppc64-inline-asm.c
  clang/test/CodeGen/RISCV/riscv-inline-asm.c
  clang/test/CodeGen/RISCV/rvv-intrinsics-handcrafted/vwrite-csr.c
  clang/test/CodeGen/X86/fma-builtins-constrained.c
  clang/test/CodeGen/X86/ms-x86-intrinsics.c
  clang/test/CodeGen/aarch64-bf16-ldst-intrinsics.c
  clang/test/CodeGen/aarch64-ls64-inline-asm.c
  clang/test/CodeGen/aarch64-sve2-intrinsics/acle_sve2_ldnt1.c
  clang/test/CodeGen/aarch64-sve2-intrinsics/acle_sve2_ldnt1sb.c
  clang/test/CodeGen/aarch64-sve2-intrinsics/acle_sve2_ldnt1sh.c
  clang/test/CodeGen/aarch64-sve2-intrinsics/acle_sve2_ldnt1sw.c
  clang/test/CodeGen/aarch64-sve2-intrinsics/acle_sve2_ldnt1ub.c
  clang/test/CodeGen/aarch64-sve2-intrinsics/acle_sve2_ldnt1uh.c
  clang/test/CodeGen/aarch64-sve2-intrinsics/acle_sve2_ldnt1uw.c
  clang/test/CodeGen/aarch64-sve2-intrinsics/acle_sve2_stnt1.c
  clang/test/CodeGen/aarch64-sve2-intrinsics/acle_sve2_stnt1b.c
  clang/test/CodeGen/aarch64-sve2-intrinsics/acle_sve2_stnt1h.c
  clang/test/CodeGen/aarch64-sve2-intrinsics/acle_sve2_stnt1w.c
  clang/test/CodeGen/aarch64-sve2-intrinsics/acle_sve2_whilege.c
  clang/test/CodeGen/aarch64-sve2-intrinsics/acle_sve2_whilegt.c
  clang/test/CodeGen/aarch64-sve2-intrinsics/acle_sve2_whilerw-bfloat.c
  clang/test/CodeGen/aarch64-sve2-intrinsics/acle_sve2_whilerw.c
  clang/test/CodeGen/aarch64-sve2-intrinsics/acle_sve2_whilewr-bfloat.c
  clang/test/CodeGen/aarch64-sve2-intrinsics/acle_sve2_whilewr.c
  
clang/test/CodeGen/aarch64_neon_sve_bridge_intrinsics/acle_neon_sve_bridge_dup_neonq.c
  
clang/test/CodeGen/aarch64_neon_sve_bridge_intrinsics/acle_neon_sve_bridge_get_neonq.c
  
clang/test/CodeGen/aarch64_neon_sve_bridge_intrinsics/acle_neon_sve_bridge_set_neonq.c
  clang/test/CodeGen/attr-arm-sve-vector-bits-cast.c
  clang/test/CodeGen/attr-riscv-rvv-vector-bits-call.c
  clang/test/CodeGen/attr-riscv-rvv-vector-bits-cast.c
  clang/test/CodeGen/msp430-builtins.c
  clang/test/CodeGen/nofpclass.c
  clang/test/Headers/wasm.c
  llvm/include/llvm/Transforms/Utils/InferCallsiteAttrs.h
  llvm/lib/Transforms/IPO/FunctionAttrs.cpp
  llvm/lib/Transforms/Utils/CMakeLists.txt
  llvm/lib/Transforms/Utils/InferCallsiteAttrs.cpp
  llvm/test/Other/cgscc-devirt-iteration.ll
  llvm/test/Transforms/FunctionAttrs/nonnull.ll
  llvm/test/Transforms/FunctionAttrs/readattrs.ll
  llvm/test/Transforms/FunctionAttrs/willreturn-callsites.ll
  llvm/test/Transforms/MergeFunc/mergefunc-preserve-debug-info.ll
  llvm/test/Transforms/PhaseOrdering/X86/loop-idiom-vs-indvars.ll
  llvm/test/Transforms/PhaseOrdering/memset-tail.ll

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

[PATCH] D152226: [FunctionAttrs] Propagate some func/arg/ret attributes from caller to callsite (WIP)

[Noah Goldstein via Phabricator via cfe-commits]

goldstein.w.n added a comment.

ping.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152226

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

[PATCH] D152226: [FunctionAttrs] Propagate some func/arg/ret attributes from caller to callsite (WIP)

[Noah Goldstein via Phabricator via cfe-commits]

goldstein.w.n updated this revision to Diff 529201.
goldstein.w.n added a comment.

Add header comments/license.
Use enum for kMaybe, kYes, kNo


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152226

Files:
  clang/test/CodeGen/LoongArch/inline-asm-constraints.c
  clang/test/CodeGen/LoongArch/inline-asm-operand-modifiers.c
  clang/test/CodeGen/LoongArch/intrinsic-la32.c
  clang/test/CodeGen/LoongArch/intrinsic-la64.c
  clang/test/CodeGen/PowerPC/builtins-ppc-build-pair-mma.c
  clang/test/CodeGen/PowerPC/builtins-ppc-pair-mma.c
  
clang/test/CodeGen/PowerPC/builtins-ppc-xlcompat-LoadReseve-StoreCond-64bit-only.c
  clang/test/CodeGen/PowerPC/builtins-ppc-xlcompat-LoadReseve-StoreCond.c
  clang/test/CodeGen/PowerPC/ppc64-inline-asm.c
  clang/test/CodeGen/RISCV/riscv-inline-asm.c
  clang/test/CodeGen/RISCV/rvv-intrinsics-handcrafted/vwrite-csr.c
  clang/test/CodeGen/X86/fma-builtins-constrained.c
  clang/test/CodeGen/X86/ms-x86-intrinsics.c
  clang/test/CodeGen/aarch64-bf16-ldst-intrinsics.c
  clang/test/CodeGen/aarch64-ls64-inline-asm.c
  clang/test/CodeGen/aarch64-sve2-intrinsics/acle_sve2_ldnt1.c
  clang/test/CodeGen/aarch64-sve2-intrinsics/acle_sve2_ldnt1sb.c
  clang/test/CodeGen/aarch64-sve2-intrinsics/acle_sve2_ldnt1sh.c
  clang/test/CodeGen/aarch64-sve2-intrinsics/acle_sve2_ldnt1sw.c
  clang/test/CodeGen/aarch64-sve2-intrinsics/acle_sve2_ldnt1ub.c
  clang/test/CodeGen/aarch64-sve2-intrinsics/acle_sve2_ldnt1uh.c
  clang/test/CodeGen/aarch64-sve2-intrinsics/acle_sve2_ldnt1uw.c
  clang/test/CodeGen/aarch64-sve2-intrinsics/acle_sve2_stnt1.c
  clang/test/CodeGen/aarch64-sve2-intrinsics/acle_sve2_stnt1b.c
  clang/test/CodeGen/aarch64-sve2-intrinsics/acle_sve2_stnt1h.c
  clang/test/CodeGen/aarch64-sve2-intrinsics/acle_sve2_stnt1w.c
  clang/test/CodeGen/aarch64-sve2-intrinsics/acle_sve2_whilege.c
  clang/test/CodeGen/aarch64-sve2-intrinsics/acle_sve2_whilegt.c
  clang/test/CodeGen/aarch64-sve2-intrinsics/acle_sve2_whilerw-bfloat.c
  clang/test/CodeGen/aarch64-sve2-intrinsics/acle_sve2_whilerw.c
  clang/test/CodeGen/aarch64-sve2-intrinsics/acle_sve2_whilewr-bfloat.c
  clang/test/CodeGen/aarch64-sve2-intrinsics/acle_sve2_whilewr.c
  
clang/test/CodeGen/aarch64_neon_sve_bridge_intrinsics/acle_neon_sve_bridge_dup_neonq.c
  
clang/test/CodeGen/aarch64_neon_sve_bridge_intrinsics/acle_neon_sve_bridge_get_neonq.c
  
clang/test/CodeGen/aarch64_neon_sve_bridge_intrinsics/acle_neon_sve_bridge_set_neonq.c
  clang/test/CodeGen/attr-arm-sve-vector-bits-cast.c
  clang/test/CodeGen/attr-riscv-rvv-vector-bits-call.c
  clang/test/CodeGen/attr-riscv-rvv-vector-bits-cast.c
  clang/test/CodeGen/msp430-builtins.c
  clang/test/CodeGen/nofpclass.c
  clang/test/Headers/wasm.c
  llvm/include/llvm/Transforms/Utils/InferCallsiteAttrs.h
  llvm/lib/Transforms/IPO/FunctionAttrs.cpp
  llvm/lib/Transforms/Utils/CMakeLists.txt
  llvm/lib/Transforms/Utils/InferCallsiteAttrs.cpp
  llvm/test/Other/cgscc-devirt-iteration.ll
  llvm/test/Transforms/FunctionAttrs/nonnull.ll
  llvm/test/Transforms/FunctionAttrs/readattrs.ll
  llvm/test/Transforms/FunctionAttrs/willreturn-callsites.ll
  llvm/test/Transforms/MergeFunc/mergefunc-preserve-debug-info.ll
  llvm/test/Transforms/PhaseOrdering/X86/loop-idiom-vs-indvars.ll
  llvm/test/Transforms/PhaseOrdering/memset-tail.ll

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

[PATCH] D152226: [FunctionAttrs] Propagate some func/arg/ret attributes from caller to callsite (WIP)

[Noah Goldstein via Phabricator via cfe-commits]

goldstein.w.n added inline comments.



Comment at: libc/CMakeLists.txt:22
+# that clang will use it.
+if(LLVM_LIBC_MAKE_DISCOVERABLE)
+  if(LLVM_ENABLE_PER_TARGET_RUNTIME_DIR)

sivachandra wrote:
> Likely out of date and not required for this patch?
woops, left that in from the bug earlier.
Fixed.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152226

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

[PATCH] D152226: [FunctionAttrs] Propagate some func/arg/ret attributes from caller to callsite (WIP)

[Noah Goldstein via Phabricator via cfe-commits]

goldstein.w.n updated this revision to Diff 528707.
goldstein.w.n marked an inline comment as done.
goldstein.w.n added a comment.

Remove libc changes


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152226

Files:
  clang/test/CodeGen/LoongArch/inline-asm-constraints.c
  clang/test/CodeGen/LoongArch/inline-asm-operand-modifiers.c
  clang/test/CodeGen/LoongArch/intrinsic-la32.c
  clang/test/CodeGen/LoongArch/intrinsic-la64.c
  clang/test/CodeGen/PowerPC/builtins-ppc-build-pair-mma.c
  clang/test/CodeGen/PowerPC/builtins-ppc-pair-mma.c
  
clang/test/CodeGen/PowerPC/builtins-ppc-xlcompat-LoadReseve-StoreCond-64bit-only.c
  clang/test/CodeGen/PowerPC/builtins-ppc-xlcompat-LoadReseve-StoreCond.c
  clang/test/CodeGen/PowerPC/ppc64-inline-asm.c
  clang/test/CodeGen/RISCV/riscv-inline-asm.c
  clang/test/CodeGen/RISCV/rvv-intrinsics-handcrafted/vwrite-csr.c
  clang/test/CodeGen/X86/fma-builtins-constrained.c
  clang/test/CodeGen/X86/ms-x86-intrinsics.c
  clang/test/CodeGen/aarch64-bf16-ldst-intrinsics.c
  clang/test/CodeGen/aarch64-ls64-inline-asm.c
  clang/test/CodeGen/aarch64-sve2-intrinsics/acle_sve2_ldnt1.c
  clang/test/CodeGen/aarch64-sve2-intrinsics/acle_sve2_ldnt1sb.c
  clang/test/CodeGen/aarch64-sve2-intrinsics/acle_sve2_ldnt1sh.c
  clang/test/CodeGen/aarch64-sve2-intrinsics/acle_sve2_ldnt1sw.c
  clang/test/CodeGen/aarch64-sve2-intrinsics/acle_sve2_ldnt1ub.c
  clang/test/CodeGen/aarch64-sve2-intrinsics/acle_sve2_ldnt1uh.c
  clang/test/CodeGen/aarch64-sve2-intrinsics/acle_sve2_ldnt1uw.c
  clang/test/CodeGen/aarch64-sve2-intrinsics/acle_sve2_stnt1.c
  clang/test/CodeGen/aarch64-sve2-intrinsics/acle_sve2_stnt1b.c
  clang/test/CodeGen/aarch64-sve2-intrinsics/acle_sve2_stnt1h.c
  clang/test/CodeGen/aarch64-sve2-intrinsics/acle_sve2_stnt1w.c
  clang/test/CodeGen/aarch64-sve2-intrinsics/acle_sve2_whilege.c
  clang/test/CodeGen/aarch64-sve2-intrinsics/acle_sve2_whilegt.c
  clang/test/CodeGen/aarch64-sve2-intrinsics/acle_sve2_whilerw-bfloat.c
  clang/test/CodeGen/aarch64-sve2-intrinsics/acle_sve2_whilerw.c
  clang/test/CodeGen/aarch64-sve2-intrinsics/acle_sve2_whilewr-bfloat.c
  clang/test/CodeGen/aarch64-sve2-intrinsics/acle_sve2_whilewr.c
  
clang/test/CodeGen/aarch64_neon_sve_bridge_intrinsics/acle_neon_sve_bridge_dup_neonq.c
  
clang/test/CodeGen/aarch64_neon_sve_bridge_intrinsics/acle_neon_sve_bridge_get_neonq.c
  
clang/test/CodeGen/aarch64_neon_sve_bridge_intrinsics/acle_neon_sve_bridge_set_neonq.c
  clang/test/CodeGen/attr-arm-sve-vector-bits-cast.c
  clang/test/CodeGen/attr-riscv-rvv-vector-bits-call.c
  clang/test/CodeGen/attr-riscv-rvv-vector-bits-cast.c
  clang/test/CodeGen/msp430-builtins.c
  clang/test/CodeGen/nofpclass.c
  clang/test/Headers/wasm.c
  llvm/include/llvm/Transforms/Utils/InferCallsiteAttrs.h
  llvm/lib/Transforms/IPO/FunctionAttrs.cpp
  llvm/lib/Transforms/Utils/CMakeLists.txt
  llvm/lib/Transforms/Utils/InferCallsiteAttrs.cpp
  llvm/test/Other/cgscc-devirt-iteration.ll
  llvm/test/Transforms/FunctionAttrs/nonnull.ll
  llvm/test/Transforms/FunctionAttrs/readattrs.ll
  llvm/test/Transforms/FunctionAttrs/willreturn-callsites.ll
  llvm/test/Transforms/MergeFunc/mergefunc-preserve-debug-info.ll
  llvm/test/Transforms/PhaseOrdering/X86/loop-idiom-vs-indvars.ll
  llvm/test/Transforms/PhaseOrdering/memset-tail.ll

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

[PATCH] D152226: [FunctionAttrs] Propagate some func/arg/ret attributes from caller to callsite (WIP)

[Noah Goldstein via Phabricator via cfe-commits]

goldstein.w.n added a comment.

llvm-test-suite passes with this patch enabled.
But note: **There currently some failing clang tests**.
Most of them look benign (just need to propagate a few attributes to a few 
thousand places),
but I was hoping there is a way to auto-regenerate them before going through 
laboriously by hand.
All the ones that can be auto regenerated I updated + a few by hand before 
thinking better of it.
Currently the list of failing tests in the llvm-project with all projects 
enabled using `ninja check-all` is:

  Failed Tests (52):
Clang :: CodeGen/SystemZ/builtins-systemz-zvector-constrained.c
Clang :: CodeGen/SystemZ/builtins-systemz-zvector.c
Clang :: CodeGen/SystemZ/builtins-systemz-zvector2-constrained.c
Clang :: CodeGen/SystemZ/builtins-systemz-zvector2.c
Clang :: CodeGen/SystemZ/builtins-systemz-zvector3-constrained.c
Clang :: CodeGen/SystemZ/builtins-systemz-zvector3.c
Clang :: CodeGen/SystemZ/systemz-inline-asm.c
Clang :: CodeGen/aarch64-neon-vcmla.c
Clang :: CodeGen/aarch64-sme-intrinsics/acle_sme_ld1.c
Clang :: CodeGen/aarch64-sme-intrinsics/acle_sme_ld1_vnum.c
Clang :: CodeGen/aarch64-sme-intrinsics/acle_sme_st1.c
Clang :: CodeGen/aarch64-sme-intrinsics/acle_sme_st1_vnum.c
Clang :: CodeGen/aarch64-sve-acle-__ARM_FEATURE_SVE_VECTOR_OPERATORS.c
Clang :: CodeGen/aarch64-sve-acle-__ARM_FEATURE_SVE_VECTOR_OPERATORS.cpp
Clang :: CodeGen/arm-cmse.c
Clang :: CodeGen/arm64-mte.c
Clang :: CodeGen/builtins-multiprecision.c
Clang :: CodeGen/builtins-wasm.c
Clang :: CodeGen/cfi-icall-cross-dso.c
Clang :: CodeGen/fp-contract-on-pragma.cpp
Clang :: CodeGen/fp-contract-pragma.cpp
Clang :: CodeGen/fp-strictfp-exp.cpp
Clang :: CodeGen/inline-asm-aarch64-flag-output.c
Clang :: CodeGen/inline-asm-x86-flag-output.c
Clang :: CodeGen/ms-intrinsics-other.c
Clang :: CodeGen/ms-intrinsics.c
Clang :: CodeGen/neon-crypto.c
Clang :: CodeGenCXX/RelativeVTablesABI/dynamic-cast.cpp
Clang :: CodeGenCXX/microsoft-abi-dynamic-cast.cpp
Clang :: CodeGenCXX/microsoft-abi-typeid.cpp
Clang :: CodeGenCXX/sizeof-unwind-exception.cpp
Clang :: CodeGenObjC/synchronized.m
Clang :: CodeGenObjCXX/exceptions-legacy.mm
Clang :: CodeGenOpenCL/builtins-amdgcn-dl-insts-gfx11.cl
Clang :: CodeGenOpenCL/builtins-amdgcn-dl-insts.cl
Clang :: CodeGenOpenCL/builtins-amdgcn-fp8.cl
Clang :: CodeGenOpenCL/builtins-amdgcn-gfx10.cl
Clang :: CodeGenOpenCL/builtins-amdgcn-gfx11.cl
Clang :: CodeGenOpenCL/builtins-amdgcn-gfx9.cl
Clang :: CodeGenOpenCL/builtins-amdgcn-mfma.cl
Clang :: CodeGenOpenCL/builtins-amdgcn-vi.cl
Clang :: CodeGenOpenCL/builtins-amdgcn-wmma-w32.cl
Clang :: CodeGenOpenCL/builtins-amdgcn-wmma-w64.cl
Clang :: CodeGenOpenCL/builtins-amdgcn.cl
Clang :: CodeGenOpenCL/builtins-f16.cl
Clang :: CodeGenOpenCL/builtins-generic-amdgcn.cl
Clang :: CodeGenOpenCL/single-precision-constant.cl
Clang :: Headers/__clang_hip_cmath.hip
Clang :: Headers/__clang_hip_math.hip
Clang :: Headers/__clang_hip_math_ocml_rounded_ops.hip
Clang :: Headers/ms-arm64-intrin.cpp
Clang :: OpenMP/bug57757.cpp

Is there a way to autoregen them, some of them need a few thousand changes 
(just adding `noundef` for the most part) that hopefully doesn't need to be 
done by hand.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152226

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

[PATCH] D152226: [FunctionAttrs] Propagate some func/arg/ret attributes from caller to callsite (WIP)

[Noah Goldstein via Phabricator via cfe-commits]

goldstein.w.n created this revision.
goldstein.w.n added reviewers: nikic, fhahn, jdoerfert, arsenm.
Herald added subscribers: libc-commits, luke, pmatos, asb, ormris, StephenFan, 
frasercrmck, luismarques, apazos, sameer.abuasal, pengfei, s.egerton, Jim, 
jocewei, PkmX, the_o, brucehoult, MartinMosbeck, rogfer01, edward-jones, 
zzheng, jrtc27, niosHD, sabuasal, simoncook, johnrusso, rbar, kbarton, 
hiraditya, sbc100, nemanjai.
Herald added projects: libc-project, All.
goldstein.w.n requested review of this revision.
Herald added subscribers: llvm-commits, cfe-commits, pcwang-thead, MaskRay, 
aheejin, wdng.
Herald added a reviewer: sstefan1.
Herald added projects: clang, LLVM.

This is the consolidation of D151644  and 
D151943  moved from
InstCombine to FunctionAttrs. This is based on discussion in the above
patches as well as D152081  (Attributor). 
This patch was written in a
way so it can have an immediate impact in currently active passes
(FunctionAttrs), but should be easy to port elsewhere (Attributor or
Inliner) if that makes more sense later on.

Some function attributes imply the attribute for all/some instructions
in the function. These attributes can be safely propagated to
callsites within the function that are missing the attribute. This can
be useful when 1) analyzing individual instructions in a function and

2. if the original caller is later inlined, as if the attributes are

not propagated, they will be lost.

This patch implements propagation in a new class/file
`InferCallsiteAttrs` which can hypothetically be included elsewhere.

At the moment this patch infers the following:

Function Attributes:

- mustprogress
- nofree
- willreturn
- All memory attributes (readnone, readonly, writeonly, argmem, etc...)
  - The memory attributes are only propagated IFF the set of pointers available 
to the callsite is the same as the set available outside the caller (i.e no 
local memory arguments from alloca or local malloc like functions).

Argument Attributes:

- noundef
- nonnull
- nofree
- readnone
- readonly
- writeonly
- nocapture
  - nocapture is only propagated IFF the set of pointers available to the 
callsite is the same as the set available outside the caller and its guranteed 
that between the callsite and function return, the state of any capture 
pointers will not change (so the nocaptured gurantee of the caller has been met 
by the instruction preceding the callsite and will not changed).

Argument are only propagated to callsite arguments that are also function
arguments, but not derived values.

Return Attributes:

- noundef
- nonnull

Return attributes are only propagated if the callsite's return value
is used as the caller's return and execution is guranteed to pass from
callsite to return.

The compile time hit of this for -O3 and -O3+thinLTO is ~[.02, .37]%
regression. Proper LTO, however, has more significant regressions (up
to 3.92%):
https://llvm-compile-time-tracker.com/compare.php?from=94407e1bba9807193afde61c56b6125c0fc0b1d1&to=79feb6e78b818e33ec69abdc58c5f713d691554f&stat=instructions:u


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D152226

Files:
  clang/test/CodeGen/LoongArch/inline-asm-constraints.c
  clang/test/CodeGen/LoongArch/inline-asm-operand-modifiers.c
  clang/test/CodeGen/LoongArch/intrinsic-la32.c
  clang/test/CodeGen/LoongArch/intrinsic-la64.c
  clang/test/CodeGen/PowerPC/builtins-ppc-build-pair-mma.c
  clang/test/CodeGen/PowerPC/builtins-ppc-pair-mma.c
  
clang/test/CodeGen/PowerPC/builtins-ppc-xlcompat-LoadReseve-StoreCond-64bit-only.c
  clang/test/CodeGen/PowerPC/builtins-ppc-xlcompat-LoadReseve-StoreCond.c
  clang/test/CodeGen/PowerPC/ppc64-inline-asm.c
  clang/test/CodeGen/RISCV/riscv-inline-asm.c
  clang/test/CodeGen/RISCV/rvv-intrinsics-handcrafted/vwrite-csr.c
  clang/test/CodeGen/X86/fma-builtins-constrained.c
  clang/test/CodeGen/X86/ms-x86-intrinsics.c
  clang/test/CodeGen/aarch64-bf16-ldst-intrinsics.c
  clang/test/CodeGen/aarch64-ls64-inline-asm.c
  clang/test/CodeGen/aarch64-sve2-intrinsics/acle_sve2_ldnt1.c
  clang/test/CodeGen/aarch64-sve2-intrinsics/acle_sve2_ldnt1sb.c
  clang/test/CodeGen/aarch64-sve2-intrinsics/acle_sve2_ldnt1sh.c
  clang/test/CodeGen/aarch64-sve2-intrinsics/acle_sve2_ldnt1sw.c
  clang/test/CodeGen/aarch64-sve2-intrinsics/acle_sve2_ldnt1ub.c
  clang/test/CodeGen/aarch64-sve2-intrinsics/acle_sve2_ldnt1uh.c
  clang/test/CodeGen/aarch64-sve2-intrinsics/acle_sve2_ldnt1uw.c
  clang/test/CodeGen/aarch64-sve2-intrinsics/acle_sve2_stnt1.c
  clang/test/CodeGen/aarch64-sve2-intrinsics/acle_sve2_stnt1b.c
  clang/test/CodeGen/aarch64-sve2-intrinsics/acle_sve2_stnt1h.c
  clang/test/CodeGen/aarch64-sve2-intrinsics/acle_sve2_stnt1w.c
  clang/test/CodeGen/aarch64-sve2-intrinsics/acle_sve2_whilege.c
  clang/test/CodeGen/aarch64-sve2-intrinsics/acle_sve2_whilegt.c
  clang/test/CodeGen/aarch64-sve2-intrinsics/acle_sve2_wh

[PATCH] D150278: [Headers][doc] Add "shift" intrinsic descriptions to avx2intrin.h

[Noah Goldstein via Phabricator via cfe-commits]

goldstein.w.n accepted this revision.
goldstein.w.n added a comment.
This revision is now accepted and ready to land.

LGTM.


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

https://reviews.llvm.org/D150278

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

[PATCH] D149205: [Headers][doc] Add "gather" intrinsic descriptions to avx2intrin.h

[Noah Goldstein via Phabricator via cfe-commits]

goldstein.w.n accepted this revision.
goldstein.w.n added a comment.
This revision is now accepted and ready to land.

LGTM. I'm not a maintainer so wait a day to push so others can take a look.




Comment at: clang/lib/Headers/avx2intrin.h:789
 
-
 static __inline__ __m128i __DEFAULT_FN_ATTRS128

Remove extrenous change?


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

https://reviews.llvm.org/D149205

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

[PATCH] D148653: [Header][doc] Add/revise MONITOR/MWAIT[X] descriptions

[Noah Goldstein via Phabricator via cfe-commits]

goldstein.w.n added a comment.

LGTM.

I'm not a maintainer to please wait a day or so to push so that others can 
review.




Comment at: clang/lib/Headers/pmmintrin.h:278
 ///the monitor event pending state. Data stored in the monitored address
 ///range causes the processor to exit the pending state.
 ///

interrupts too. Might as well add that if updating the comments.



Comment at: clang/lib/Headers/pmmintrin.h:291
 /// \param __hints
-///Optional hints for the monitoring state, which may vary by processor.
+///Optional hints for the monitoring state, which can vary by processor.
 static __inline__ void __DEFAULT_FN_ATTRS

out of curiosity, why "may" -> "can"?


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

https://reviews.llvm.org/D148653

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

[PATCH] D142862: [Support] change StringMap hash function from djbHash to xxHash

[Noah Goldstein via Phabricator via cfe-commits]

goldstein.w.n added a comment.

FWIW, if the only use if hashtables `xxhash` has a slight perf bug in 
`XXH3_len_129to240_128b` and `XXH3_len_129to240_64b` where it will repeat the 
last the block if length is a factor of `32`/`16` respectively.
`XXH3_len_129to240_128b`:

- https://github.com/Cyan4973/xxHash/blob/dev/xxhash.h#L3981

`XXH3_len_129to240_64b`:

- https://github.com/Cyan4973/xxHash/blob/dev/xxhash.h#L5877

It's not fixed b.c `xxhash` needs to have a stable result for compression, but 
for hashtable it might be worth fixing in the internal llvm version?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142862

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

[PATCH] D137181: [clang-format] Don't use 'PPIndentWidth' inside multi-line macros

[Noah Goldstein via Phabricator via cfe-commits]

goldstein.w.n added a comment.

In D137181#3938942 , @owenpan wrote:

> In D137181#3938913 , @goldstein.w.n 
> wrote:
>
>> Anything left for me todo (can't imagine I have push access).
>
> See here . We 
> will need your name and email.

Added `[clang-format]` tag to commit message.

Name: Noah Goldstein
email: goldstein@gmail.com

but that is already in the commit info.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137181

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

[PATCH] D137181: [clang-format] Don't use 'PPIndentWidth' inside multi-line macros

[Noah Goldstein via Phabricator via cfe-commits]

goldstein.w.n updated this revision to Diff 476732.
goldstein.w.n added a comment.



1. Updating D137181 : [clang-format] Don't 
use 'PPIndentWidth' inside multi-line macros #
2. Enter a brief description of the changes included in this update.
3. The first line is used as subject, next lines as comment. #
4. If you intended to create a new revision, use:
5. $ arc diff --create

Add tag to commit message


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137181

Files:
  clang/lib/Format/TokenAnnotator.cpp
  clang/lib/Format/TokenAnnotator.h
  clang/lib/Format/UnwrappedLineFormatter.cpp
  clang/lib/Format/UnwrappedLineParser.cpp
  clang/lib/Format/UnwrappedLineParser.h
  clang/unittests/Format/FormatTest.cpp

Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -5059,6 +5059,213 @@
"  int y = 0;\n"
"}",
style);
+
+  style.IndentPPDirectives = FormatStyle::PPDIS_None;
+  verifyFormat("#ifdef foo\n"
+   "#define bar() \\\n"
+   "if (A) {  \\\n"
+   "B();  \\\n"
+   "} \\\n"
+   "C();\n"
+   "#endif",
+   style);
+  verifyFormat("if (emacs) {\n"
+   "#ifdef is\n"
+   "#define lit   \\\n"
+   "if (af) { \\\n"
+   "return duh(); \\\n"
+   "}\n"
+   "#endif\n"
+   "}",
+   style);
+  verifyFormat("#if abc\n"
+   "#ifdef foo\n"
+   "#define bar()\\\n"
+   "if (A) { \\\n"
+   "if (B) { \\\n"
+   "C(); \\\n"
+   "}\\\n"
+   "}\\\n"
+   "D();\n"
+   "#endif\n"
+   "#endif",
+   style);
+  verifyFormat("#ifndef foo\n"
+   "#define foo\n"
+   "if (emacs) {\n"
+   "#ifdef is\n"
+   "#define lit   \\\n"
+   "if (af) { \\\n"
+   "return duh(); \\\n"
+   "}\n"
+   "#endif\n"
+   "}\n"
+   "#endif",
+   style);
+  verifyFormat("#if 1\n"
+   "#define X  \\\n"
+   "{  \\\n"
+   "x; \\\n"
+   "x; \\\n"
+   "}\n"
+   "#endif",
+   style);
+  verifyFormat("#define X  \\\n"
+   "{  \\\n"
+   "x; \\\n"
+   "x; \\\n"
+   "}",
+   style);
+
+  style.PPIndentWidth = 2;
+  verifyFormat("#ifdef foo\n"
+   "#define bar() \\\n"
+   "if (A) {  \\\n"
+   "B();  \\\n"
+   "} \\\n"
+   "C();\n"
+   "#endif",
+   style);
+  style.IndentWidth = 8;
+  verifyFormat("#ifdef foo\n"
+   "#define bar()\\\n"
+   "if (A) { \\\n"
+   "B(); \\\n"
+   "}\\\n"
+   "C();\n"
+   "#endif",
+   style);
+
+  style.IndentWidth = 1;
+  style.PPIndentWidth = 4;
+  verifyFormat("#if 1\n"
+   "#define X \\\n"
+   " {\\\n"
+   "  x;  \\\n"
+   "  x;  \\\n"
+   " }\n"
+   "#endif",
+   style);
+  verifyFormat("#define X \\\n"
+   " {\\\n"
+   "  x;  \\\n"
+   "  x;  \\\n"
+   " }",
+   style);
+
+  style.IndentWidth = 4;
+  style.PPIndentWidth = 1;
+  style.IndentPPDirectives = FormatStyle::PPDIS_AfterHash;
+  verifyFormat("#ifdef foo\n"
+   "# define bar() \\\n"
+   " if (A) {  \\\n"
+   " B();  \\\n"
+   " } \\\n"
+   " C();\n"
+   "#endif",
+   style);
+  verifyFormat("#if abc\n"
+   "# ifdef foo\n"
+   "#  define bar()\\\n"
+   "  if (A) { \\\n"
+   "  if (B) { \\\n"
+   "  C(); \\\n"
+   "  }\\\n"
+   "  }\\\n"
+   "  D();\n"
+   "# endif\n"
+   "#endif",
+   style);
+  verifyFormat("#ifndef foo\n"
+   "#define foo\n"
+   "if (emacs) {\n"
+   "#ifdef is\n"
+   "# define lit   \

[PATCH] D137181: [clang-format] Don't use 'PPIndentWidth' inside multi-line macros

[Noah Goldstein via Phabricator via cfe-commits]

goldstein.w.n marked an inline comment as done.
goldstein.w.n added a comment.

In D137181#3938904 , @owenpan wrote:

> LGTM

Awesome! Thanks for sticking with it through all these revisions :)

Anything left for me todo (can't imagine I have push access).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137181

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

[PATCH] D137181: [clang-format] Don't use 'PPIndentWidth' inside multi-line macros

[Noah Goldstein via Phabricator via cfe-commits]

goldstein.w.n marked 2 inline comments as done.
goldstein.w.n added inline comments.



Comment at: clang/lib/Format/UnwrappedLineParser.cpp:838
+  // If this changes PPLevel needs to be used for get correct indentation.
+  assert(!Line.InMacroBody && !InPPDirective);
   return Line.Level * Style.IndentWidth + Length <= ColumnLimit;

owenpan wrote:
> ```
> error: use of undeclared identifier 'InPPDirective'
>   assert(!Line.InMacroBody && !InPPDirective);
>^
> ```
> Also, can you break it into two assertions as suggested so that we know which 
> one failed even without a debugger?
Fixed. Sorry. Had only been rebuilting formattests. Figured everything else 
must have a dep. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137181

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

[PATCH] D137181: [clang-format] Don't use 'PPIndentWidth' inside multi-line macros

[Noah Goldstein via Phabricator via cfe-commits]

goldstein.w.n marked an inline comment as done.
goldstein.w.n added a comment.

In D137181#3938771 , @owenpan wrote:

> In D137181#3935951 , @owenpan wrote:
>
>> In D137181#3935856 , 
>> @goldstein.w.n wrote:
>>
>>> I could remove either the `PPDIS_BeforeHash` or `PPDIS_AfterHash` though as 
>>> the they really no independent logic in this commit.
>>
>> Yes, please. Or better yet, alternate a little bit between the two.
>
> My bad. I meant splitting the test cases between the two. Can you regroup 
> them (lines 5137-5258) as follows?
>
>   style.IndentPPDirectives = FormatStyle::PPDIS_AfterHash;
>   style.IndentWidth = 4;
>   style.PPIndentWidth = 1;
>   verifyFormat("#ifdef foo\n"
>"# define bar() \\\n"
>" if (A) {  \\\n"
>" B();  \\\n"
>" } \\\n"
>" C();\n"
>"#endif",
>style);
>   verifyFormat("#if abc\n"
>"# ifdef foo\n"
>"#  define bar()\\\n"
>"  if (A) { \\\n"
>"  if (B) { \\\n"
>"  C(); \\\n"
>"  }\\\n"
>"  }\\\n"
>"  D();\n"
>"# endif\n"
>"#endif",
>style);
>   verifyFormat("#ifndef foo\n"
>"#define foo\n"
>"if (emacs) {\n"
>"#ifdef is\n"
>"# define lit   \\\n"
>" if (af) { \\\n"
>" return duh(); \\\n"
>" }\n"
>"#endif\n"
>"}\n"
>"#endif",
>style);
>   verifyFormat("#define X  \\\n"
>"{  \\\n"
>"x; \\\n"
>"x; \\\n"
>"}",
>style);
>   
>   style.IndentWidth = 8;
>   style.PPIndentWidth = 2;
>   verifyFormat("#ifdef foo\n"
>"#  define bar()\\\n"
>"  if (A) { \\\n"
>"  B(); \\\n"
>"  }\\\n"
>"  C();\n"
>"#endif",
>style);
>   
>   style.IndentWidth = 1;
>   style.PPIndentWidth = 4;
>   verifyFormat("#define X \\\n"
>" {\\\n"
>"  x;  \\\n"
>"  x;  \\\n"
>" }",
>style);
>   
>   style.IndentPPDirectives = FormatStyle::PPDIS_BeforeHash;
>   style.IndentWidth = 4;
>   style.PPIndentWidth = 1;
>   verifyFormat("if (emacs) {\n"
>"#ifdef is\n"
>" #define lit   \\\n"
>" if (af) { \\\n"
>" return duh(); \\\n"
>" }\n"
>"#endif\n"
>"}",
>style);
>   verifyFormat("#if abc\n"
>" #ifdef foo\n"
>"  #define bar() \\\n"
>"  if (A) {  \\\n"
>"  B();  \\\n"
>"  } \\\n"
>"  C();\n"
>" #endif\n"
>"#endif",
>style);
>   verifyFormat("#if 1\n"
>" #define X  \\\n"
>" {  \\\n"
>" x; \\\n"
>" x; \\\n"
>" }\n"
>"#endif",
>style);
>   
>   style.PPIndentWidth = 2;
>   verifyFormat("#ifdef foo\n"
>"  #define bar() \\\n"
>"  if (A) {  \\\n"
>"  B();  \\\n"
>"  } \\\n"
>"  C();\n"
>"#endif",
>style);
>   
>   style.IndentWidth = 1;
>   style.PPIndentWidth = 4;
>   verifyFormat("#if 1\n"
>"#define X \\\n"
>" {\\\n"
>"  x;  \\\n"
>"  x;  \\\n"
>" }\n"
>"#endif",
>style);

Yes done.




Comment at: clang/lib/Format/UnwrappedLineParser.cpp:1281
+  Line->PPLevel = PPBranchLevel + (IncludeGuard == IG_Defined ? 0 : 1);
   Line->InMacroBody = true;
 

owenpan wrote:
> In case `PPLevel` is negative.
Done.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137181

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

[PATCH] D137181: [clang-format] Don't use 'PPIndentWidth' inside multi-line macros

[Noah Goldstein via Phabricator via cfe-commits]

goldstein.w.n updated this revision to Diff 476654.
goldstein.w.n added a comment.



1. Updating D137181 : [clang-format] Don't 
use 'PPIndentWidth' inside multi-line macros #
2. Enter a brief description of the changes included in this update.
3. The first line is used as subject, next lines as comment. #
4. If you intended to create a new revision, use:
5. $ arc diff --create

De-interleave test cases.
Cleanup asserts.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137181

Files:
  clang/lib/Format/TokenAnnotator.cpp
  clang/lib/Format/TokenAnnotator.h
  clang/lib/Format/UnwrappedLineFormatter.cpp
  clang/lib/Format/UnwrappedLineParser.cpp
  clang/lib/Format/UnwrappedLineParser.h
  clang/unittests/Format/FormatTest.cpp

Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -5040,6 +5040,213 @@
"  int y = 0;\n"
"}",
style);
+
+  style.IndentPPDirectives = FormatStyle::PPDIS_None;
+  verifyFormat("#ifdef foo\n"
+   "#define bar() \\\n"
+   "if (A) {  \\\n"
+   "B();  \\\n"
+   "} \\\n"
+   "C();\n"
+   "#endif",
+   style);
+  verifyFormat("if (emacs) {\n"
+   "#ifdef is\n"
+   "#define lit   \\\n"
+   "if (af) { \\\n"
+   "return duh(); \\\n"
+   "}\n"
+   "#endif\n"
+   "}",
+   style);
+  verifyFormat("#if abc\n"
+   "#ifdef foo\n"
+   "#define bar()\\\n"
+   "if (A) { \\\n"
+   "if (B) { \\\n"
+   "C(); \\\n"
+   "}\\\n"
+   "}\\\n"
+   "D();\n"
+   "#endif\n"
+   "#endif",
+   style);
+  verifyFormat("#ifndef foo\n"
+   "#define foo\n"
+   "if (emacs) {\n"
+   "#ifdef is\n"
+   "#define lit   \\\n"
+   "if (af) { \\\n"
+   "return duh(); \\\n"
+   "}\n"
+   "#endif\n"
+   "}\n"
+   "#endif",
+   style);
+  verifyFormat("#if 1\n"
+   "#define X  \\\n"
+   "{  \\\n"
+   "x; \\\n"
+   "x; \\\n"
+   "}\n"
+   "#endif",
+   style);
+  verifyFormat("#define X  \\\n"
+   "{  \\\n"
+   "x; \\\n"
+   "x; \\\n"
+   "}",
+   style);
+
+  style.PPIndentWidth = 2;
+  verifyFormat("#ifdef foo\n"
+   "#define bar() \\\n"
+   "if (A) {  \\\n"
+   "B();  \\\n"
+   "} \\\n"
+   "C();\n"
+   "#endif",
+   style);
+  style.IndentWidth = 8;
+  verifyFormat("#ifdef foo\n"
+   "#define bar()\\\n"
+   "if (A) { \\\n"
+   "B(); \\\n"
+   "}\\\n"
+   "C();\n"
+   "#endif",
+   style);
+
+  style.IndentWidth = 1;
+  style.PPIndentWidth = 4;
+  verifyFormat("#if 1\n"
+   "#define X \\\n"
+   " {\\\n"
+   "  x;  \\\n"
+   "  x;  \\\n"
+   " }\n"
+   "#endif",
+   style);
+  verifyFormat("#define X \\\n"
+   " {\\\n"
+   "  x;  \\\n"
+   "  x;  \\\n"
+   " }",
+   style);
+
+  style.IndentWidth = 4;
+  style.PPIndentWidth = 1;
+  style.IndentPPDirectives = FormatStyle::PPDIS_AfterHash;
+  verifyFormat("#ifdef foo\n"
+   "# define bar() \\\n"
+   " if (A) {  \\\n"
+   " B();  \\\n"
+   " } \\\n"
+   " C();\n"
+   "#endif",
+   style);
+  verifyFormat("#if abc\n"
+   "# ifdef foo\n"
+   "#  define bar()\\\n"
+   "  if (A) { \\\n"
+   "  if (B) { \\\n"
+   "  C(); \\\n"
+   "  }\\\n"
+   "  }\\\n"
+   "  D();\n"
+   "# endif\n"
+   "#endif",
+   style);
+  verifyFormat("#ifndef foo\n"
+   "#define foo\n"
+   "if (emacs) {\n"
+   "#ifdef is\n"
+   "# defin

[PATCH] D137181: [clang-format] Don't use 'PPIndentWidth' inside multi-line macros

[Noah Goldstein via Phabricator via cfe-commits]

goldstein.w.n added a comment.

In D137181#3935951 , @owenpan wrote:

> In D137181#3935856 , @goldstein.w.n 
> wrote:
>
>> In D137181#3935752 , @owenpan 
>> wrote:
>>
>>> Please mark review comments as done if you have addressed them. Can you 
>>> also clean up the test cases, removing overlapping/redundant ones, making 
>>> sure a test case doesn't end with a newline (e.g., line 5380), etc?
>>
>> Regarding newlines. I have new line before changing the style. Otherwise 
>> done. (let me know if you want me to remove those).
>
> Sorry, it's line 5379 and several other places that have an ending newline. 
> (Just search for `\n",`.) You only need to remove them in the new test cases. 
> We can fix the existing ones in a separate NFC patch.

Done. Sorry I had thought you mean the newline in the file between the test 
cases.

>> Regarding removing redundant test cases. I think I've had each of them fail 
>> independently at some point (although many when I was doing the previous 
>> PPLevel tracking) so I wouldn't call any of them truly redundant.
>
> I know, but IMO some of the failures (e.g. the `switch` statement) during 
> development shouldn't automatically go in. This file already has a humongous 
> size. Maybe do the best you can here.
>
>> I could remove either the `PPDIS_BeforeHash` or `PPDIS_AfterHash` though as 
>> the they really no independent logic in this commit.
>
> Yes, please. Or better yet, alternate a little bit between the two.

Done.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137181

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

[PATCH] D137181: [clang-format] Don't use 'PPIndentWidth' inside multi-line macros

[Noah Goldstein via Phabricator via cfe-commits]

goldstein.w.n updated this revision to Diff 476528.
goldstein.w.n added a comment.



1. Updating D137181 : [clang-format] Don't 
use 'PPIndentWidth' inside multi-line macros #
2. Enter a brief description of the changes included in this update.
3. The first line is used as subject, next lines as comment. #
4. If you intended to create a new revision, use:
5. $ arc diff --create

Cleanup tests file


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137181

Files:
  clang/lib/Format/TokenAnnotator.cpp
  clang/lib/Format/TokenAnnotator.h
  clang/lib/Format/UnwrappedLineFormatter.cpp
  clang/lib/Format/UnwrappedLineParser.cpp
  clang/lib/Format/UnwrappedLineParser.h
  clang/unittests/Format/FormatTest.cpp

Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -5040,6 +5040,222 @@
"  int y = 0;\n"
"}",
style);
+
+  style.IndentPPDirectives = FormatStyle::PPDIS_None;
+  verifyFormat("#ifdef foo\n"
+   "#define bar() \\\n"
+   "if (A) {  \\\n"
+   "B();  \\\n"
+   "} \\\n"
+   "C();\n"
+   "#endif",
+   style);
+  verifyFormat("if (emacs) {\n"
+   "#ifdef is\n"
+   "#define lit   \\\n"
+   "if (af) { \\\n"
+   "return duh(); \\\n"
+   "}\n"
+   "#endif\n"
+   "}",
+   style);
+  verifyFormat("#if abc\n"
+   "#ifdef foo\n"
+   "#define bar()\\\n"
+   "if (A) { \\\n"
+   "if (B) { \\\n"
+   "C(); \\\n"
+   "}\\\n"
+   "}\\\n"
+   "D();\n"
+   "#endif\n"
+   "#endif",
+   style);
+  verifyFormat("#ifndef foo\n"
+   "#define foo\n"
+   "if (emacs) {\n"
+   "#ifdef is\n"
+   "#define lit   \\\n"
+   "if (af) { \\\n"
+   "return duh(); \\\n"
+   "}\n"
+   "#endif\n"
+   "}\n"
+   "#endif",
+   style);
+  verifyFormat("#if 1\n"
+   "#define X  \\\n"
+   "{  \\\n"
+   "x; \\\n"
+   "x; \\\n"
+   "}\n"
+   "#endif",
+   style);
+  verifyFormat("#define X  \\\n"
+   "{  \\\n"
+   "x; \\\n"
+   "x; \\\n"
+   "}",
+   style);
+
+  style.PPIndentWidth = 2;
+  verifyFormat("#ifdef foo\n"
+   "#define bar() \\\n"
+   "if (A) {  \\\n"
+   "B();  \\\n"
+   "} \\\n"
+   "C();\n"
+   "#endif",
+   style);
+  style.IndentWidth = 8;
+  verifyFormat("#ifdef foo\n"
+   "#define bar()\\\n"
+   "if (A) { \\\n"
+   "B(); \\\n"
+   "}\\\n"
+   "C();\n"
+   "#endif",
+   style);
+
+  style.IndentWidth = 1;
+  style.PPIndentWidth = 4;
+  verifyFormat("#if 1\n"
+   "#define X \\\n"
+   " {\\\n"
+   "  x;  \\\n"
+   "  x;  \\\n"
+   " }\n"
+   "#endif",
+   style);
+  verifyFormat("#define X \\\n"
+   " {\\\n"
+   "  x;  \\\n"
+   "  x;  \\\n"
+   " }",
+   style);
+
+  style.IndentWidth = 4;
+  style.PPIndentWidth = 1;
+  style.IndentPPDirectives = FormatStyle::PPDIS_AfterHash;
+  verifyFormat("#ifdef foo\n"
+   "# define bar() \\\n"
+   " if (A) {  \\\n"
+   " B();  \\\n"
+   " } \\\n"
+   " C();\n"
+   "#endif",
+   style);
+
+  style.IndentPPDirectives = FormatStyle::PPDIS_BeforeHash;
+  verifyFormat("if (emacs) {\n"
+   "#ifdef is\n"
+   " #define lit   \\\n"
+   " if (af) { \\\n"
+   " return duh(); \\\n"
+   " }\n"
+   "#endif\n"
+   "}",
+   style);
+
+  style.IndentPPDirectives = FormatStyle::PPDIS_AfterHash;
+  verifyFormat("#if abc\n"
+   "# ifdef foo\n"
+   "#  define bar()\\\n"
+   "  if (A) { \\\n"
+   "  

[PATCH] D137181: [clang-format] Don't use 'PPIndentWidth' inside multi-line macros

[Noah Goldstein via Phabricator via cfe-commits]

goldstein.w.n added a comment.

In D137181#3935752 , @owenpan wrote:

> Please mark review comments as done if you have addressed them. Can you also 
> clean up the test cases, removing overlapping/redundant ones, making sure a 
> test case doesn't end with a newline (e.g., line 5380), etc?

Regarding newlines. I have new line before changing the style. Otherwise done. 
(let me know if you want me to remove those).

Regarding removing redundant test cases. I think I've had each of them fail 
independently at some point (although many when I was doing the previous 
PPLevel tracking) so I wouldn't call any of them truly redundant.
I could remove either the `PPDIS_BeforeHash` or `PPDIS_AfterHash` though as the 
they really no independent logic in this commit.

Thoughts?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137181

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

[PATCH] D137181: [clang-format] Don't use 'PPIndentWidth' inside multi-line macros

[Noah Goldstein via Phabricator via cfe-commits]

goldstein.w.n added inline comments.



Comment at: clang/lib/Format/UnwrappedLineFormatter.cpp:69-75
+  if (Line.InMacroBody) {
+Indent = (Line.PPLevel + 1) * PPIndentWidth;
+Indent += (Line.Level - Line.PPLevel - 1) * Style.IndentWidth;
+Indent += Style.IndentWidth - PPIndentWidth;
+  } else {
+Indent = Line.Level * PPIndentWidth;
+  }

owenpan wrote:
> To make it simpler and clearer.
Done.



Comment at: clang/lib/Format/UnwrappedLineParser.cpp:838
+  // If this changes PPLevel needs to be used for get correct indentation.
+  assert(!(Line.InMacroBody && InPPDirective));
   return Line.Level * Style.IndentWidth + Length <= ColumnLimit;

owenpan wrote:
> Shouldn't it be `assert(!Line.InMacroBody && !InPPDirective)` instead? You 
> can also assert `Line.PPLevel == 0`.
I guess the goal was to assert condition where `PPLevel` would be used to 
calculate if a line might find (if both `InMacroBody` and `InPPDirective`) are 
true. But your assert should also works fine so fixed.



Comment at: clang/lib/Format/UnwrappedLineParser.cpp:1280
+
+  if (Style.IndentPPDirectives != FormatStyle::PPDIS_None)
+Line->PPLevel = PPBranchLevel + (IncludeGuard == IG_Defined ? 0 : 1);

owenpan wrote:
> We don't need the `if` statement anymore. Actually, it's better to always set 
> `PPLevel` for `InMacroBody` lines.
Fixed.



Comment at: clang/unittests/Format/FormatTest.cpp:5382-5384
+
+  style.IndentWidth = 4;
+  style.PPIndentWidth = 1;

owenpan wrote:
> Please delete.
Fixed.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137181

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

[PATCH] D137181: [clang-format] Don't use 'PPIndentWidth' inside multi-line macros

[Noah Goldstein via Phabricator via cfe-commits]

goldstein.w.n updated this revision to Diff 476337.
goldstein.w.n added a comment.



1. Updating D137181 : [clang-format] Don't 
use 'PPIndentWidth' inside multi-line macros #
2. Enter a brief description of the changes included in this update.
3. The first line is used as subject, next lines as comment. #
4. If you intended to create a new revision, use:
5. $ arc diff --create

Fix some nits.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137181

Files:
  clang/lib/Format/TokenAnnotator.cpp
  clang/lib/Format/TokenAnnotator.h
  clang/lib/Format/UnwrappedLineFormatter.cpp
  clang/lib/Format/UnwrappedLineParser.cpp
  clang/lib/Format/UnwrappedLineParser.h
  clang/unittests/Format/FormatTest.cpp

Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -5040,6 +5040,344 @@
"  int y = 0;\n"
"}",
style);
+
+  style.IndentPPDirectives = FormatStyle::PPDIS_None;
+  verifyFormat("#ifdef X\n"
+   "#define Y  \\\n"
+   "switch (Y) {   \\\n"
+   "case 0:\\\n"
+   "break; \\\n"
+   "case 1:\\\n"
+   "break; \\\n"
+   "}  \\\n"
+   "Z();\n"
+   "#endif",
+   style);
+  verifyFormat("#ifdef foo\n"
+   "#define bar() \\\n"
+   "if (A) {  \\\n"
+   "B();  \\\n"
+   "} \\\n"
+   "C();\n"
+   "#endif",
+   style);
+  verifyFormat("if (emacs) {\n"
+   "#ifdef is\n"
+   "#define lit   \\\n"
+   "if (af) { \\\n"
+   "return duh(); \\\n"
+   "}\n"
+   "#endif\n"
+   "}",
+   style);
+  verifyFormat("#if abc\n"
+   "#ifdef foo\n"
+   "#define bar()\\\n"
+   "if (A) { \\\n"
+   "if (B) { \\\n"
+   "C(); \\\n"
+   "}\\\n"
+   "}\\\n"
+   "D();\n"
+   "#endif\n"
+   "#endif",
+   style);
+  verifyFormat("#ifndef foo\n"
+   "#define foo\n"
+   "if (emacs) {\n"
+   "#ifdef is\n"
+   "#define lit   \\\n"
+   "if (af) { \\\n"
+   "return duh(); \\\n"
+   "}\n"
+   "#endif\n"
+   "}\n"
+   "#endif",
+   style);
+  verifyFormat("#if 1\n"
+   "#define X  \\\n"
+   "{  \\\n"
+   "x; \\\n"
+   "x; \\\n"
+   "}\n"
+   "#endif",
+   style);
+  verifyFormat("#define X  \\\n"
+   "{  \\\n"
+   "x; \\\n"
+   "x; \\\n"
+   "}\n",
+   style);
+
+  style.PPIndentWidth = 2;
+  verifyFormat("#ifdef foo\n"
+   "#define bar() \\\n"
+   "if (A) {  \\\n"
+   "B();  \\\n"
+   "} \\\n"
+   "C();\n"
+   "#endif",
+   style);
+  style.IndentWidth = 8;
+  verifyFormat("#ifdef foo\n"
+   "#define bar()\\\n"
+   "if (A) { \\\n"
+   "B(); \\\n"
+   "}\\\n"
+   "C();\n"
+   "#endif",
+   style);
+
+  style.IndentWidth = 1;
+  style.PPIndentWidth = 4;
+  verifyFormat("#if 1\n"
+   "#define X \\\n"
+   " {\\\n"
+   "  x;  \\\n"
+   "  x;  \\\n"
+   " }\n"
+   "#endif",
+   style);
+  verifyFormat("#define X \\\n"
+   " {\\\n"
+   "  x;  \\\n"
+   "  x;  \\\n"
+   " }\n",
+   style);
+
+  style.IndentWidth = 4;
+  style.PPIndentWidth = 1;
+  style.IndentPPDirectives = FormatStyle::PPDIS_AfterHash;
+  verifyFormat("#ifdef X\n"
+   "# define Y  \\\n"
+   " switch (Y) {   \\\n"
+   " case 0:\\\n"
+   " break; \\\n"
+   " case 1:\\\n"
+   " break; \\\n"
+   " }  \\\n"
+   " Z();\n"
+   "#endif",
+   style);
+  verifyFormat("#ifdef foo\n"
+   

[PATCH] D137181: [clang-format] Don't use 'PPIndentWidth' inside multi-line macros

[Noah Goldstein via Phabricator via cfe-commits]

goldstein.w.n added a comment.

In D137181#3926848 , @owenpan wrote:

> In D137181#3924002 , @sstwcw wrote:
>
>> Can you make `TokenAnnotator::printDebugInfo` print `PPLevel`?
>
> @goldstein.w.n can you add it as follows?
>
>   $ git diff TokenAnnotator.cpp
>   diff --git a/clang/lib/Format/TokenAnnotator.cpp 
> b/clang/lib/Format/TokenAnnotator.cpp
>   index 75570552146c..536472e9d136 100644
>   --- a/clang/lib/Format/TokenAnnotator.cpp
>   +++ b/clang/lib/Format/TokenAnnotator.cpp
>   @@ -5093,8 +5093,9 @@ bool TokenAnnotator::canBreakBefore(const 
> AnnotatedLine &Line,
>}
>
>void TokenAnnotator::printDebugInfo(const AnnotatedLine &Line) const {
>   -  llvm::errs() << "AnnotatedTokens(L=" << Line.Level << ", T=" << Line.Type
>   -   << ", C=" << Line.IsContinuation << "):\n";
>   +  llvm::errs() << "AnnotatedTokens(L=" << Line.Level << ", P=" << 
> Line.PPLevel
>   +   << ", T=" << Line.Type << ", C=" << Line.IsContinuation
>   +   << "):\n";
>  const FormatToken *Tok = Line.First;
>  while (Tok) {
>llvm::errs() << " M=" << Tok->MustBreakBefore

Done.

>> Since you changed the rules for indentation in `UnwrappedLineFormatter`, do 
>> you also need to change `UnwrappedLineParser::mightFitOnOneLine`?
>
> `mightFitOnOneLine` is not called on PPDirective lines, but it's a good idea 
> to add an assertion.

Added assert.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137181

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

[PATCH] D137181: [clang-format] Don't use 'PPIndentWidth' inside multi-line macros

[Noah Goldstein via Phabricator via cfe-commits]

goldstein.w.n updated this revision to Diff 476013.
goldstein.w.n marked 2 inline comments as done.
goldstein.w.n added a comment.



1. Updating D137181 : [clang-format] Don't 
use 'PPIndentWidth' inside multi-line macros #
2. Enter a brief description of the changes included in this update.
3. The first line is used as subject, next lines as comment. #
4. If you intended to create a new revision, use:
5. $ arc diff --create

Add PPLevel to debug printout + assert not in macro in mightFitOnOneLine


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137181

Files:
  clang/lib/Format/TokenAnnotator.cpp
  clang/lib/Format/TokenAnnotator.h
  clang/lib/Format/UnwrappedLineFormatter.cpp
  clang/lib/Format/UnwrappedLineParser.cpp
  clang/lib/Format/UnwrappedLineParser.h
  clang/unittests/Format/FormatTest.cpp

Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -5040,6 +5040,348 @@
"  int y = 0;\n"
"}",
style);
+
+  style.IndentPPDirectives = FormatStyle::PPDIS_None;
+  verifyFormat("#ifdef X\n"
+   "#define Y  \\\n"
+   "switch (Y) {   \\\n"
+   "case 0:\\\n"
+   "break; \\\n"
+   "case 1:\\\n"
+   "break; \\\n"
+   "}  \\\n"
+   "Z();\n"
+   "#endif",
+   style);
+  verifyFormat("#ifdef foo\n"
+   "#define bar() \\\n"
+   "if (A) {  \\\n"
+   "B();  \\\n"
+   "} \\\n"
+   "C();\n"
+   "#endif",
+   style);
+  verifyFormat("if (emacs) {\n"
+   "#ifdef is\n"
+   "#define lit   \\\n"
+   "if (af) { \\\n"
+   "return duh(); \\\n"
+   "}\n"
+   "#endif\n"
+   "}",
+   style);
+  verifyFormat("#if abc\n"
+   "#ifdef foo\n"
+   "#define bar()\\\n"
+   "if (A) { \\\n"
+   "if (B) { \\\n"
+   "C(); \\\n"
+   "}\\\n"
+   "}\\\n"
+   "D();\n"
+   "#endif\n"
+   "#endif",
+   style);
+  verifyFormat("#ifndef foo\n"
+   "#define foo\n"
+   "if (emacs) {\n"
+   "#ifdef is\n"
+   "#define lit   \\\n"
+   "if (af) { \\\n"
+   "return duh(); \\\n"
+   "}\n"
+   "#endif\n"
+   "}\n"
+   "#endif",
+   style);
+  verifyFormat("#if 1\n"
+   "#define X  \\\n"
+   "{  \\\n"
+   "x; \\\n"
+   "x; \\\n"
+   "}\n"
+   "#endif",
+   style);
+  verifyFormat("#define X  \\\n"
+   "{  \\\n"
+   "x; \\\n"
+   "x; \\\n"
+   "}\n",
+   style);
+
+  style.PPIndentWidth = 2;
+  verifyFormat("#ifdef foo\n"
+   "#define bar() \\\n"
+   "if (A) {  \\\n"
+   "B();  \\\n"
+   "} \\\n"
+   "C();\n"
+   "#endif",
+   style);
+  style.IndentWidth = 8;
+  verifyFormat("#ifdef foo\n"
+   "#define bar()\\\n"
+   "if (A) { \\\n"
+   "B(); \\\n"
+   "}\\\n"
+   "C();\n"
+   "#endif",
+   style);
+
+  style.IndentWidth = 1;
+  style.PPIndentWidth = 4;
+  verifyFormat("#if 1\n"
+   "#define X \\\n"
+   " {\\\n"
+   "  x;  \\\n"
+   "  x;  \\\n"
+   " }\n"
+   "#endif",
+   style);
+  verifyFormat("#define X \\\n"
+   " {\\\n"
+   "  x;  \\\n"
+   "  x;  \\\n"
+   " }\n",
+   style);
+
+  style.IndentWidth = 4;
+  style.PPIndentWidth = 1;
+
+  style.IndentPPDirectives = FormatStyle::PPDIS_AfterHash;
+  verifyFormat("#ifdef X\n"
+   "# define Y  \\\n"
+   " switch (Y) {   \\\n"
+   " case 0:\\\n"
+   " break; \\\n"
+   " case 1:\\\n"
+   " break; \\\n"
+   " }  \\\n"
+   "  

[PATCH] D137181: [clang-format] Don't use 'PPIndentWidth' inside multi-line macros

[Noah Goldstein via Phabricator via cfe-commits]

goldstein.w.n marked 2 inline comments as done.
goldstein.w.n added inline comments.



Comment at: clang/lib/Format/UnwrappedLineFormatter.cpp:69
   (Style.PPIndentWidth >= 0) ? Style.PPIndentWidth : Style.IndentWidth;
-  Indent = Line.Level * IndentWidth + AdditionalIndent;
+  if (Line.InMacroBody && Line.PPLevel != 0 &&
+  static_cast(Line.PPLevel) < Line.Level) {

sstwcw wrote:
> What is this test for?
> 
> ```
> clang-format -style='{IndentPPDirective: BeforeHash, PPIndentWidth: 2, 
> IndentWidth: 4, ColumnLimit: 40}'
> 
> actual:
> #define X  \
>   {\
> x; \
> x; \
>   }
> 
> expected:
> #define X  \
> {  \
> x; \
> x; \
> }
> ```
You're right, unneeded. Fixed and added test for it.



Comment at: clang/lib/Format/UnwrappedLineFormatter.cpp:73
+Indent += (Line.Level - Line.PPLevel - 1) * Style.IndentWidth;
+if (PPIndentWidth < Style.IndentWidth)
+  Indent += Style.IndentWidth - PPIndentWidth;

sstwcw wrote:
> What is this for?
> 
> ```
> clang-format -style='{IndentPPDirective: BeforeHash, PPIndentWidth: 4, 
> IndentWidth: 1, ColumnLimit: 40}'
> 
> actual:
> #if X
> #define X  \
> {  \
>  x;\
>  x;\
> }
> #endif
> 
> expected:
> #if X
> #define X  \
>  { \
>   x;   \
>   x;   \
>  }
> #endif
> ```
You're right, unneeded. Fixed and added test for it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137181

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

[PATCH] D137181: [clang-format] Don't use 'PPIndentWidth' inside multi-line macros

[Noah Goldstein via Phabricator via cfe-commits]

goldstein.w.n added a comment.

In D137181#3923921 , @owenpan wrote:

> I think we are close to the finishing line. Can you revisit the change to the 
> formatter and clean it up? For example, casting `PPLevel` to `unsigned` is 
> redundant now. IMO you can simply the change too.

Cleaned up the logic a bit.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137181

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

[PATCH] D137181: [clang-format] Don't use 'PPIndentWidth' inside multi-line macros

[Noah Goldstein via Phabricator via cfe-commits]

goldstein.w.n updated this revision to Diff 475050.
goldstein.w.n added a comment.



1. Updating D137181 : [clang-format] Don't 
use 'PPIndentWidth' inside multi-line macros #
2. Enter a brief description of the changes included in this update.
3. The first line is used as subject, next lines as comment. #
4. If you intended to create a new revision, use:
5. $ arc diff --create

Cleanup logic in Formatter


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137181

Files:
  clang/lib/Format/TokenAnnotator.h
  clang/lib/Format/UnwrappedLineFormatter.cpp
  clang/lib/Format/UnwrappedLineParser.cpp
  clang/lib/Format/UnwrappedLineParser.h
  clang/unittests/Format/FormatTest.cpp

Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -5040,6 +5040,348 @@
"  int y = 0;\n"
"}",
style);
+
+  style.IndentPPDirectives = FormatStyle::PPDIS_None;
+  verifyFormat("#ifdef X\n"
+   "#define Y  \\\n"
+   "switch (Y) {   \\\n"
+   "case 0:\\\n"
+   "break; \\\n"
+   "case 1:\\\n"
+   "break; \\\n"
+   "}  \\\n"
+   "Z();\n"
+   "#endif",
+   style);
+  verifyFormat("#ifdef foo\n"
+   "#define bar() \\\n"
+   "if (A) {  \\\n"
+   "B();  \\\n"
+   "} \\\n"
+   "C();\n"
+   "#endif",
+   style);
+  verifyFormat("if (emacs) {\n"
+   "#ifdef is\n"
+   "#define lit   \\\n"
+   "if (af) { \\\n"
+   "return duh(); \\\n"
+   "}\n"
+   "#endif\n"
+   "}",
+   style);
+  verifyFormat("#if abc\n"
+   "#ifdef foo\n"
+   "#define bar()\\\n"
+   "if (A) { \\\n"
+   "if (B) { \\\n"
+   "C(); \\\n"
+   "}\\\n"
+   "}\\\n"
+   "D();\n"
+   "#endif\n"
+   "#endif",
+   style);
+  verifyFormat("#ifndef foo\n"
+   "#define foo\n"
+   "if (emacs) {\n"
+   "#ifdef is\n"
+   "#define lit   \\\n"
+   "if (af) { \\\n"
+   "return duh(); \\\n"
+   "}\n"
+   "#endif\n"
+   "}\n"
+   "#endif",
+   style);
+  verifyFormat("#if 1\n"
+   "#define X  \\\n"
+   "{  \\\n"
+   "x; \\\n"
+   "x; \\\n"
+   "}\n"
+   "#endif",
+   style);
+  verifyFormat("#define X  \\\n"
+   "{  \\\n"
+   "x; \\\n"
+   "x; \\\n"
+   "}\n",
+   style);
+
+  style.PPIndentWidth = 2;
+  verifyFormat("#ifdef foo\n"
+   "#define bar() \\\n"
+   "if (A) {  \\\n"
+   "B();  \\\n"
+   "} \\\n"
+   "C();\n"
+   "#endif",
+   style);
+  style.IndentWidth = 8;
+  verifyFormat("#ifdef foo\n"
+   "#define bar()\\\n"
+   "if (A) { \\\n"
+   "B(); \\\n"
+   "}\\\n"
+   "C();\n"
+   "#endif",
+   style);
+
+  style.IndentWidth = 1;
+  style.PPIndentWidth = 4;
+  verifyFormat("#if 1\n"
+   "#define X \\\n"
+   " {\\\n"
+   "  x;  \\\n"
+   "  x;  \\\n"
+   " }\n"
+   "#endif",
+   style);
+  verifyFormat("#define X \\\n"
+   " {\\\n"
+   "  x;  \\\n"
+   "  x;  \\\n"
+   " }\n",
+   style);
+
+  style.IndentWidth = 4;
+  style.PPIndentWidth = 1;
+
+  style.IndentPPDirectives = FormatStyle::PPDIS_AfterHash;
+  verifyFormat("#ifdef X\n"
+   "# define Y  \\\n"
+   " switch (Y) {   \\\n"
+   " case 0:\\\n"
+   " break; \\\n"
+   " case 1:\\\n"
+   " break; \\\n"
+   " }  \\\n"
+   " Z();\n"
+   "#endif",
+   style);
+  verifyFormat("#ifdef foo\n"
+   "# define bar() \\\n"
+ 

[PATCH] D137181: [clang-format] Don't use 'PPIndentWidth' inside multi-line macros

[Noah Goldstein via Phabricator via cfe-commits]

goldstein.w.n added a comment.

In D137181#3923434 , @owenpan wrote:

> In D137181#3918715 , @goldstein.w.n 
> wrote:
>
>> In D137181#3918673 , @owenpan 
>> wrote:
>>
>>> Below is how I defined `PPLevel`:
>>>
>>>   $ git diff UnwrappedLineParser.h
>>>   diff --git a/clang/lib/Format/UnwrappedLineParser.h 
>>> b/clang/lib/Format/UnwrappedLineParser.h
>>>   index b9b106bcc89a..a234f6852e0c 100644
>>>   --- a/clang/lib/Format/UnwrappedLineParser.h
>>>   +++ b/clang/lib/Format/UnwrappedLineParser.h
>>>   @@ -43,6 +43,9 @@ struct UnwrappedLine {
>>>
>>>  /// The indent level of the \c UnwrappedLine.
>>>  unsigned Level;
>>>   +  /// The \c PPBranchLevel (adjusted for header guards) of the macro 
>>> definition
>>>   +  /// this line belongs to.
>>>   +  unsigned PPLevel;
>>>
>>>  /// Whether this \c UnwrappedLine is part of a preprocessor directive.
>>>  bool InPPDirective;
>>>   @@ -358,7 +361,7 @@ struct UnwrappedLineNode {
>>>};
>>>
>>>inline UnwrappedLine::UnwrappedLine()
>>>   -: Level(0), InPPDirective(false), InPragmaDirective(false),
>>>   +: Level(0), PPLevel(0), InPPDirective(false), 
>>> InPragmaDirective(false),
>>>  InMacroBody(false), MustBeDeclaration(false),
>>>  MatchingOpeningBlockLineIndex(kInvalidIndex) {}
>>>
>>>
>>> Conceptually, I think it's more accurate to make `PPLevel` to mean the PP 
>>> branching level of the `#define` line, not the first line of the macro 
>>> body. IMO it may simplify the changes you made to the formatter.
>>
>> Hmm? Not sure what you mean.
>
> Does the comments above `unsigned PPLevel;` in the above `git diff` output 
> help? Anyway, you already addressed my comment by decreasing `PPLevel` by 1, 
> so it's ok now.

Got it. Is the patch okay?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137181

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

[PATCH] D137181: [clang-format] Don't use 'PPIndentWidth' inside multi-line macros

[Noah Goldstein via Phabricator via cfe-commits]

goldstein.w.n added a comment.

In D137181#3918673 , @owenpan wrote:

> In D137181#3918117 , @goldstein.w.n 
> wrote:
>
 maybe you did something different?
>>>
>>> Here is what I did:
>>>
>>>   $ git diff UnwrappedLineParser.cpp
>>>   diff --git a/clang/lib/Format/UnwrappedLineParser.cpp 
>>> b/clang/lib/Format/UnwrappedLineParser.cpp
>>>   index 25d9018fa109..ab3b9c53ee54 100644
>>>   --- a/clang/lib/Format/UnwrappedLineParser.cpp
>>>   +++ b/clang/lib/Format/UnwrappedLineParser.cpp
>>>   @@ -197,6 +197,7 @@ public:
>>>PreBlockLine = std::move(Parser.Line);
>>>Parser.Line = std::make_unique();
>>>Parser.Line->Level = PreBlockLine->Level;
>>>   +Parser.Line->PPLevel = PreBlockLine->PPLevel;
>>>Parser.Line->InPPDirective = PreBlockLine->InPPDirective;
>>>Parser.Line->InMacroBody = PreBlockLine->InMacroBody;
>>>  }
>>>   @@ -1274,6 +1275,10 @@ void UnwrappedLineParser::parsePPDefine() {
>>>  addUnwrappedLine();
>>>  ++Line->Level;
>>>  Line->InMacroBody = true;
>>>   +  if (Style.IndentPPDirectives != FormatStyle::PPDIS_None) {
>>>   +Line->PPLevel =
>>>   +IncludeGuard == IG_Defined ? PPBranchLevel : PPBranchLevel + 1;
>>>   +  }
>>>
>>>  // Errors during a preprocessor directive can only affect the layout 
>>> of the
>>>  // preprocessor directive, and thus we ignore them. An alternative 
>>> approach
>>
>> You're right this works (but off by one). Updated patch.
>
> It was not off by 1, or else it wouldn't have worked. Below is how I defined 
> `PPLevel`:
>
>   $ git diff UnwrappedLineParser.h
>   diff --git a/clang/lib/Format/UnwrappedLineParser.h 
> b/clang/lib/Format/UnwrappedLineParser.h
>   index b9b106bcc89a..a234f6852e0c 100644
>   --- a/clang/lib/Format/UnwrappedLineParser.h
>   +++ b/clang/lib/Format/UnwrappedLineParser.h
>   @@ -43,6 +43,9 @@ struct UnwrappedLine {
>
>  /// The indent level of the \c UnwrappedLine.
>  unsigned Level;
>   +  /// The \c PPBranchLevel (adjusted for header guards) of the macro 
> definition
>   +  /// this line belongs to.
>   +  unsigned PPLevel;
>
>  /// Whether this \c UnwrappedLine is part of a preprocessor directive.
>  bool InPPDirective;
>   @@ -358,7 +361,7 @@ struct UnwrappedLineNode {
>};
>
>inline UnwrappedLine::UnwrappedLine()
>   -: Level(0), InPPDirective(false), InPragmaDirective(false),
>   +: Level(0), PPLevel(0), InPPDirective(false), InPragmaDirective(false),
>  InMacroBody(false), MustBeDeclaration(false),
>  MatchingOpeningBlockLineIndex(kInvalidIndex) {}
>
>
> Conceptually, I think it's more accurate to make `PPLevel` to mean the PP 
> branching level of the `#define` line, not the first line of the macro body. 
> IMO it may simplify the changes you made to the formatter.

Hmm? Not sure what you mean.

> BTW you still need to change `PPLevel` to `unsigned` and initialize it as 
> requested in D137181#inline-1328385 
> .

Fixed.




Comment at: clang/lib/Format/UnwrappedLineParser.h:66
+  /// #endifPPLevel still at : 0
+  int PPLevel;
+

owenpan wrote:
> goldstein.w.n wrote:
> > owenpan wrote:
> > > You need to initialize `PPLevel` in the constructor. FWIW I'd use 
> > > `unsigned` to be consistent with `Level` above.
> > Will initialize. It's `int` because we do a less than zero check in the 
> > Formatter.
> > It's `int` because we do a less than zero check in the Formatter.
> 
> If you set `PPLevel` to `PPBranchLevel` when there is no header guard, and to 
> `PPBranchLevel + 1`otherwise, `PPLevel` can never be negative.
Sorry didnt see your reply. Fixed


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137181

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

[PATCH] D137181: [clang-format] Don't use 'PPIndentWidth' inside multi-line macros

[Noah Goldstein via Phabricator via cfe-commits]

goldstein.w.n updated this revision to Diff 474447.
goldstein.w.n added a comment.



1. Updating D137181 : [clang-format] Don't 
use 'PPIndentWidth' inside multi-line macros #
2. Enter a brief description of the changes included in this update.
3. The first line is used as subject, next lines as comment. #
4. If you intended to create a new revision, use:
5. $ arc diff --create

Make PPLevel unsigned + Initialize in constructor.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137181

Files:
  clang/lib/Format/TokenAnnotator.h
  clang/lib/Format/UnwrappedLineFormatter.cpp
  clang/lib/Format/UnwrappedLineParser.cpp
  clang/lib/Format/UnwrappedLineParser.h
  clang/unittests/Format/FormatTest.cpp

Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -5040,6 +5040,254 @@
"  int y = 0;\n"
"}",
style);
+
+  style.IndentPPDirectives = FormatStyle::PPDIS_None;
+  verifyFormat("#ifdef X\n"
+   "#define Y  \\\n"
+   "switch (Y) {   \\\n"
+   "case 0:\\\n"
+   "break; \\\n"
+   "case 1:\\\n"
+   "break; \\\n"
+   "}  \\\n"
+   "Z();\n"
+   "#endif",
+   style);
+  verifyFormat("#ifdef foo\n"
+   "#define bar() \\\n"
+   "if (A) {  \\\n"
+   "B();  \\\n"
+   "} \\\n"
+   "C();\n"
+   "#endif",
+   style);
+  verifyFormat("if (emacs) {\n"
+   "#ifdef is\n"
+   "#define lit   \\\n"
+   "if (af) { \\\n"
+   "return duh(); \\\n"
+   "}\n"
+   "#endif\n"
+   "}",
+   style);
+  verifyFormat("#if abc\n"
+   "#ifdef foo\n"
+   "#define bar()\\\n"
+   "if (A) { \\\n"
+   "if (B) { \\\n"
+   "C(); \\\n"
+   "}\\\n"
+   "}\\\n"
+   "D();\n"
+   "#endif\n"
+   "#endif",
+   style);
+  verifyFormat("#ifndef foo\n"
+   "#define foo\n"
+   "if (emacs) {\n"
+   "#ifdef is\n"
+   "#define lit   \\\n"
+   "if (af) { \\\n"
+   "return duh(); \\\n"
+   "}\n"
+   "#endif\n"
+   "}\n"
+   "#endif",
+   style);
+
+  style.PPIndentWidth = 2;
+  verifyFormat("#ifdef foo\n"
+   "#define bar() \\\n"
+   "if (A) {  \\\n"
+   "B();  \\\n"
+   "} \\\n"
+   "C();\n"
+   "#endif",
+   style);
+  style.IndentWidth = 8;
+  verifyFormat("#ifdef foo\n"
+   "#define bar()\\\n"
+   "if (A) { \\\n"
+   "B(); \\\n"
+   "}\\\n"
+   "C();\n"
+   "#endif",
+   style);
+
+  style.IndentWidth = 4;
+  style.PPIndentWidth = 1;
+
+  style.IndentPPDirectives = FormatStyle::PPDIS_AfterHash;
+  verifyFormat("#ifdef X\n"
+   "# define Y  \\\n"
+   " switch (Y) {   \\\n"
+   " case 0:\\\n"
+   " break; \\\n"
+   " case 1:\\\n"
+   " break; \\\n"
+   " }  \\\n"
+   " Z();\n"
+   "#endif",
+   style);
+  verifyFormat("#ifdef foo\n"
+   "# define bar() \\\n"
+   " if (A) {  \\\n"
+   " B();  \\\n"
+   " } \\\n"
+   " C();\n"
+   "#endif",
+   style);
+  verifyFormat("if (emacs) {\n"
+   "#ifdef is\n"
+   "# define lit   \\\n"
+   " if (af) { \\\n"
+   " return duh(); \\\n"
+   " }\n"
+   "#endif\n"
+   "}",
+   style);
+  verifyFormat("#if abc\n"
+   "# ifdef foo\n"
+   "#  define bar()\\\n"
+   "  if (A) { \\\n"
+   "  if (B) { \\\n"
+   "  C(); \\\n"
+   "  }\\\n"
+   "  }\\\n"
+   "  D();\n"
+   "# endif\n"
+

[PATCH] D137181: [clang-format] Don't use 'PPIndentWidth' inside multi-line macros

[Noah Goldstein via Phabricator via cfe-commits]

goldstein.w.n added a comment.

In D137181#3916605 , @owenpan wrote:

> In D137181#3916558 , @goldstein.w.n 
> wrote:
>
>> In D137181#3916547 , @owenpan 
>> wrote:
>>
>>> Yes, if there is a header guard. Otherwise, set `PPLevel` to `PPBranchLevel 
>>> + 1`.
>>
>> That fails alot of the tests for me.
>>
>> maybe you did something different?
>
> Here is what I did:
>
>   $ git diff UnwrappedLineParser.cpp
>   diff --git a/clang/lib/Format/UnwrappedLineParser.cpp 
> b/clang/lib/Format/UnwrappedLineParser.cpp
>   index 25d9018fa109..ab3b9c53ee54 100644
>   --- a/clang/lib/Format/UnwrappedLineParser.cpp
>   +++ b/clang/lib/Format/UnwrappedLineParser.cpp
>   @@ -197,6 +197,7 @@ public:
>PreBlockLine = std::move(Parser.Line);
>Parser.Line = std::make_unique();
>Parser.Line->Level = PreBlockLine->Level;
>   +Parser.Line->PPLevel = PreBlockLine->PPLevel;
>Parser.Line->InPPDirective = PreBlockLine->InPPDirective;
>Parser.Line->InMacroBody = PreBlockLine->InMacroBody;
>  }
>   @@ -1274,6 +1275,10 @@ void UnwrappedLineParser::parsePPDefine() {
>  addUnwrappedLine();
>  ++Line->Level;
>  Line->InMacroBody = true;
>   +  if (Style.IndentPPDirectives != FormatStyle::PPDIS_None) {
>   +Line->PPLevel =
>   +IncludeGuard == IG_Defined ? PPBranchLevel : PPBranchLevel + 1;
>   +  }
>
>  // Errors during a preprocessor directive can only affect the layout of 
> the
>  // preprocessor directive, and thus we ignore them. An alternative 
> approach

You're right this works (but off by one). Updated patch.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137181

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

[PATCH] D137181: [clang-format] Don't use 'PPIndentWidth' inside multi-line macros

[Noah Goldstein via Phabricator via cfe-commits]

goldstein.w.n updated this revision to Diff 474359.
goldstein.w.n added a comment.



1. Updating D137181 : [clang-format] Don't 
use 'PPIndentWidth' inside multi-line macros #
2. Enter a brief description of the changes included in this update.
3. The first line is used as subject, next lines as comment. #
4. If you intended to create a new revision, use:
5. $ arc diff --create

Remove many unnecessary `PPLevel` changes.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137181

Files:
  clang/lib/Format/TokenAnnotator.h
  clang/lib/Format/UnwrappedLineFormatter.cpp
  clang/lib/Format/UnwrappedLineParser.cpp
  clang/lib/Format/UnwrappedLineParser.h
  clang/unittests/Format/FormatTest.cpp

Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -5040,6 +5040,254 @@
"  int y = 0;\n"
"}",
style);
+
+  style.IndentPPDirectives = FormatStyle::PPDIS_None;
+  verifyFormat("#ifdef X\n"
+   "#define Y  \\\n"
+   "switch (Y) {   \\\n"
+   "case 0:\\\n"
+   "break; \\\n"
+   "case 1:\\\n"
+   "break; \\\n"
+   "}  \\\n"
+   "Z();\n"
+   "#endif",
+   style);
+  verifyFormat("#ifdef foo\n"
+   "#define bar() \\\n"
+   "if (A) {  \\\n"
+   "B();  \\\n"
+   "} \\\n"
+   "C();\n"
+   "#endif",
+   style);
+  verifyFormat("if (emacs) {\n"
+   "#ifdef is\n"
+   "#define lit   \\\n"
+   "if (af) { \\\n"
+   "return duh(); \\\n"
+   "}\n"
+   "#endif\n"
+   "}",
+   style);
+  verifyFormat("#if abc\n"
+   "#ifdef foo\n"
+   "#define bar()\\\n"
+   "if (A) { \\\n"
+   "if (B) { \\\n"
+   "C(); \\\n"
+   "}\\\n"
+   "}\\\n"
+   "D();\n"
+   "#endif\n"
+   "#endif",
+   style);
+  verifyFormat("#ifndef foo\n"
+   "#define foo\n"
+   "if (emacs) {\n"
+   "#ifdef is\n"
+   "#define lit   \\\n"
+   "if (af) { \\\n"
+   "return duh(); \\\n"
+   "}\n"
+   "#endif\n"
+   "}\n"
+   "#endif",
+   style);
+
+  style.PPIndentWidth = 2;
+  verifyFormat("#ifdef foo\n"
+   "#define bar() \\\n"
+   "if (A) {  \\\n"
+   "B();  \\\n"
+   "} \\\n"
+   "C();\n"
+   "#endif",
+   style);
+  style.IndentWidth = 8;
+  verifyFormat("#ifdef foo\n"
+   "#define bar()\\\n"
+   "if (A) { \\\n"
+   "B(); \\\n"
+   "}\\\n"
+   "C();\n"
+   "#endif",
+   style);
+
+  style.IndentWidth = 4;
+  style.PPIndentWidth = 1;
+
+  style.IndentPPDirectives = FormatStyle::PPDIS_AfterHash;
+  verifyFormat("#ifdef X\n"
+   "# define Y  \\\n"
+   " switch (Y) {   \\\n"
+   " case 0:\\\n"
+   " break; \\\n"
+   " case 1:\\\n"
+   " break; \\\n"
+   " }  \\\n"
+   " Z();\n"
+   "#endif",
+   style);
+  verifyFormat("#ifdef foo\n"
+   "# define bar() \\\n"
+   " if (A) {  \\\n"
+   " B();  \\\n"
+   " } \\\n"
+   " C();\n"
+   "#endif",
+   style);
+  verifyFormat("if (emacs) {\n"
+   "#ifdef is\n"
+   "# define lit   \\\n"
+   " if (af) { \\\n"
+   " return duh(); \\\n"
+   " }\n"
+   "#endif\n"
+   "}",
+   style);
+  verifyFormat("#if abc\n"
+   "# ifdef foo\n"
+   "#  define bar()\\\n"
+   "  if (A) { \\\n"
+   "  if (B) { \\\n"
+   "  C(); \\\n"
+   "  }\\\n"
+   "  }\\\n"
+   "  D();\n"
+   "# endif\n"
+

[PATCH] D137181: [clang-format] Don't use 'PPIndentWidth' inside multi-line macros

[Noah Goldstein via Phabricator via cfe-commits]

goldstein.w.n added a comment.

In D137181#3916547 , @owenpan wrote:

> In D137181#3916521 , @goldstein.w.n 
> wrote:
>
>> I think we can remove the places I set `InitialPPLevel` and `OldPPLevel` but 
>> anything else I remove causes at least one test to fail.
>>
>> What did you do? Just set `PPLevel = PPBranchLevel`?
>
> Yes, if there is a header guard. Otherwise, set `PPLevel` to `PPBranchLevel + 
> 1`.

That fails alot of the tests for me.

I have a patch attached (applyable on this) maybe you did something 
different?F25244239: v1-0001-Remove-all-other-defs.patch 



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137181

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

[PATCH] D137181: [clang-format] Don't use 'PPIndentWidth' inside multi-line macros

[Noah Goldstein via Phabricator via cfe-commits]

goldstein.w.n added a comment.

In D137181#3916505 , @owenpan wrote:

> In D137181#3916488 , @goldstein.w.n 
> wrote:
>
>> In D137181#3916474 , @owenpan 
>> wrote:
>>
>>> You can still simply the changes to UnwrappedLineParser.cpp a lot. In fact, 
>>> instead of adjusting `PPLevel` at various places, you only need to set it 
>>> once when `InMacroBody` is set.
>>
>> I don't think thats right, it needs to stay updated with the true macro 
>> depth (even though we only use `PPLevel` for formatting `InMacroBody` lines).
>
> My experiment from a few days ago was successful and also passes all the new 
> tests you just added here. If your experience told you otherwise, then 
> perhaps there are some missing test cases.

What did you do? Just set `PPLevel = PPBranchLevel`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137181

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

[PATCH] D137181: [clang-format] Don't use 'PPIndentWidth' inside multi-line macros

[Noah Goldstein via Phabricator via cfe-commits]

goldstein.w.n added a comment.

In D137181#3916474 , @owenpan wrote:

> You can still simply the changes to UnwrappedLineParser.cpp a lot. In fact, 
> instead of adjusting `PPLevel` at various places, you only need to set it 
> once when `InMacroBody` is set.

I don't think thats right, it needs to stay updated with the true macro depth 
(even though we only use `PPLevel` for formatting `InMacroBody` lines).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137181

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

[PATCH] D137181: [clang-format] Don't use 'PPIndentWidth' inside multi-line macros

[Noah Goldstein via Phabricator via cfe-commits]

goldstein.w.n added inline comments.



Comment at: clang/lib/Format/UnwrappedLineFormatter.cpp:92-94
+  if (Line.Level >= IndentForLevel.size())
+IndentForLevel.resize(Line.Level + 1, UnknownIndent ? -1 : Indent);
+}

owenpan wrote:
> Please run git-clang-format.
Will do for next submissions (thought `arc` did that automatically on 
precommit).



Comment at: clang/lib/Format/UnwrappedLineParser.cpp:112
   : Line(Line), TokenSource(TokenSource), ResetToken(ResetToken),
-PreviousLineLevel(Line.Level), PreviousTokenSource(TokenSource),
-Token(nullptr), PreviousToken(nullptr) {
+PreviousLineLevel(Line.Level), PreviousLinePPLevel(Line.PPLevel),
+PreviousTokenSource(TokenSource), Token(nullptr),

owenpan wrote:
> Do you need to add `PreviousLinePPLevel` here? If yes, can you add test cases 
> for it? Otherwise, you don't need to make any changes to `ScopedMacroState`.
It's needed.

Without:
```
Expected equality of these values:
  Expected.str()
Which is: "#ifndef foo\n#define foo\nif (emacs) {\n#ifdef is\n #define lit  
 \\\n if (af) { \\\n return duh(); \\\n 
}\n#endif\n}\n#endif"
  format(test::messUp(Code), ObjCStyle)
Which is: "#ifndef foo\n#define foo\nif (emacs) {\n#ifdef is\n #define lit  
  \\\n  if (af) { \\\n  return duh(); \\\n  
}\n#endif\n}\n#endif"
With diff:
@@ -3,8 +3,8 @@
 if (emacs) {
 #ifdef is
- #define lit   \\
- if (af) { \\
- return duh(); \\
- }
+ #define lit\\
+  if (af) { \\
+  return duh(); \\
+  }
 #endif
 }
```



Comment at: clang/lib/Format/UnwrappedLineParser.h:66
+  /// #endifPPLevel still at : 0
+  int PPLevel;
+

owenpan wrote:
> You need to initialize `PPLevel` in the constructor. FWIW I'd use `unsigned` 
> to be consistent with `Level` above.
Will initialize. It's `int` because we do a less than zero check in the 
Formatter.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137181

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

[PATCH] D137181: [clang-format] Don't use 'PPIndentWidth' inside multi-line macros

[Noah Goldstein via Phabricator via cfe-commits]

goldstein.w.n added inline comments.



Comment at: clang/lib/Format/UnwrappedLineParser.h:47
 
+  /// The preprocessor indent level of the \c UnwrappedLine.
+  int PPLevel;

sstwcw wrote:
> Please elaborate what preprocessor indent level means.
Done.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137181

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

[PATCH] D137181: [clang-format] Don't use 'PPIndentWidth' inside multi-line macros

[Noah Goldstein via Phabricator via cfe-commits]

goldstein.w.n updated this revision to Diff 474079.
goldstein.w.n added a comment.



1. Updating D137181 : [clang-format] Don't 
use 'PPIndentWidth' inside multi-line macros #
2. Enter a brief description of the changes included in this update.
3. The first line is used as subject, next lines as comment. #
4. If you intended to create a new revision, use:
5. $ arc diff --create

Elaborate in comments for PPLevel


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137181

Files:
  clang/lib/Format/TokenAnnotator.h
  clang/lib/Format/UnwrappedLineFormatter.cpp
  clang/lib/Format/UnwrappedLineParser.cpp
  clang/lib/Format/UnwrappedLineParser.h
  clang/unittests/Format/FormatTest.cpp

Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -5040,6 +5040,254 @@
"  int y = 0;\n"
"}",
style);
+
+  style.IndentPPDirectives = FormatStyle::PPDIS_None;
+  verifyFormat("#ifdef X\n"
+   "#define Y  \\\n"
+   "switch (Y) {   \\\n"
+   "case 0:\\\n"
+   "break; \\\n"
+   "case 1:\\\n"
+   "break; \\\n"
+   "}  \\\n"
+   "Z();\n"
+   "#endif",
+   style);
+  verifyFormat("#ifdef foo\n"
+   "#define bar() \\\n"
+   "if (A) {  \\\n"
+   "B();  \\\n"
+   "} \\\n"
+   "C();\n"
+   "#endif",
+   style);
+  verifyFormat("if (emacs) {\n"
+   "#ifdef is\n"
+   "#define lit   \\\n"
+   "if (af) { \\\n"
+   "return duh(); \\\n"
+   "}\n"
+   "#endif\n"
+   "}",
+   style);
+  verifyFormat("#if abc\n"
+   "#ifdef foo\n"
+   "#define bar()\\\n"
+   "if (A) { \\\n"
+   "if (B) { \\\n"
+   "C(); \\\n"
+   "}\\\n"
+   "}\\\n"
+   "D();\n"
+   "#endif\n"
+   "#endif",
+   style);
+  verifyFormat("#ifndef foo\n"
+   "#define foo\n"
+   "if (emacs) {\n"
+   "#ifdef is\n"
+   "#define lit   \\\n"
+   "if (af) { \\\n"
+   "return duh(); \\\n"
+   "}\n"
+   "#endif\n"
+   "}\n"
+   "#endif",
+   style);
+
+  style.PPIndentWidth = 2;
+  verifyFormat("#ifdef foo\n"
+   "#define bar() \\\n"
+   "if (A) {  \\\n"
+   "B();  \\\n"
+   "} \\\n"
+   "C();\n"
+   "#endif",
+   style);
+  style.IndentWidth = 8;
+  verifyFormat("#ifdef foo\n"
+   "#define bar()\\\n"
+   "if (A) { \\\n"
+   "B(); \\\n"
+   "}\\\n"
+   "C();\n"
+   "#endif",
+   style);
+
+  style.IndentWidth = 4;
+  style.PPIndentWidth = 1;
+
+  style.IndentPPDirectives = FormatStyle::PPDIS_AfterHash;
+  verifyFormat("#ifdef X\n"
+   "# define Y  \\\n"
+   " switch (Y) {   \\\n"
+   " case 0:\\\n"
+   " break; \\\n"
+   " case 1:\\\n"
+   " break; \\\n"
+   " }  \\\n"
+   " Z();\n"
+   "#endif",
+   style);
+  verifyFormat("#ifdef foo\n"
+   "# define bar() \\\n"
+   " if (A) {  \\\n"
+   " B();  \\\n"
+   " } \\\n"
+   " C();\n"
+   "#endif",
+   style);
+  verifyFormat("if (emacs) {\n"
+   "#ifdef is\n"
+   "# define lit   \\\n"
+   " if (af) { \\\n"
+   " return duh(); \\\n"
+   " }\n"
+   "#endif\n"
+   "}",
+   style);
+  verifyFormat("#if abc\n"
+   "# ifdef foo\n"
+   "#  define bar()\\\n"
+   "  if (A) { \\\n"
+   "  if (B) { \\\n"
+   "  C(); \\\n"
+   "  }\\\n"
+   "  }\\\n"
+   "  D();\n"
+   "# endif\n"
+   "#endi

[PATCH] D137181: [clang-format] Don't use 'PPIndentWidth' inside multi-line macros

[Noah Goldstein via Phabricator via cfe-commits]

goldstein.w.n added a comment.

In D137181#3914292 , @owenpan wrote:

> In D137181#3914100 , @goldstein.w.n 
> wrote:
>
>> Err I'm not sure that's right. I thought we where going for the C-code 
>> should start at the column that the 'd' in define is.
>> So it would be:
>>
>>   #define X   \
>>switch (x) {   \
>>case 0:\
>>break; \
>>default:   \
>>break; \
>>}
>
> That doesn't make sense IMO. Here is an example:
>
>   $ cat foo.cpp
>   #if X
>   #define FOO \
>   if (a)  \
>   b;
>   #endif
>   $ clang-format -style="{BasedOnStyle: Chromium, IndentWidth: 4}" foo.cpp > 
> out1
>   $ diff foo.cpp out1
>   $ clang-format -style="{BasedOnStyle: Chromium, IndentWidth: 4, 
> PPIndentWidth: 1}" foo.cpp > out2
>
> Now what should out2 look like? Well, it should be no different than out1 
> (and foo.cpp) because `IndentPPDirectives` is still `None` by default. 
> However, the current patch outputs the following instead:
>
>   #if X
>   #define FOO \
>if (a) \
>b;
>   #endif
>
> So I really think the first line of a macro definition body should be 
> indented `IndentWidth - 1` relative to the letter `d` in `define` no matter 
> what `IndentPPDirectives` and `PPIndentWidth` are set to.

Fair enough. Adding something like:

  if (PPIndentWidth != Style.IndentWidth)
Indent += Style.IndentWidth - 1;

Works for that and all existing tests pass + added a few tests with varied 
indentwidth / ppindentwidth.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137181

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

[PATCH] D137181: [clang-format] Don't use 'PPIndentWidth' inside multi-line macros

[Noah Goldstein via Phabricator via cfe-commits]

goldstein.w.n updated this revision to Diff 473917.
goldstein.w.n added a comment.



1. Updating D137181 : [clang-format] Don't 
use 'PPIndentWidth' inside multi-line macros #
2. Enter a brief description of the changes included in this update.
3. The first line is used as subject, next lines as comment. #
4. If you intended to create a new revision, use:
5. $ arc diff --create

Make code inside macros start at common `IndentWidth - 1`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137181

Files:
  clang/lib/Format/TokenAnnotator.h
  clang/lib/Format/UnwrappedLineFormatter.cpp
  clang/lib/Format/UnwrappedLineParser.cpp
  clang/lib/Format/UnwrappedLineParser.h
  clang/unittests/Format/FormatTest.cpp

Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -5040,6 +5040,254 @@
"  int y = 0;\n"
"}",
style);
+
+  style.IndentPPDirectives = FormatStyle::PPDIS_None;
+  verifyFormat("#ifdef X\n"
+   "#define Y  \\\n"
+   "switch (Y) {   \\\n"
+   "case 0:\\\n"
+   "break; \\\n"
+   "case 1:\\\n"
+   "break; \\\n"
+   "}  \\\n"
+   "Z();\n"
+   "#endif",
+   style);
+  verifyFormat("#ifdef foo\n"
+   "#define bar() \\\n"
+   "if (A) {  \\\n"
+   "B();  \\\n"
+   "} \\\n"
+   "C();\n"
+   "#endif",
+   style);
+  verifyFormat("if (emacs) {\n"
+   "#ifdef is\n"
+   "#define lit   \\\n"
+   "if (af) { \\\n"
+   "return duh(); \\\n"
+   "}\n"
+   "#endif\n"
+   "}",
+   style);
+  verifyFormat("#if abc\n"
+   "#ifdef foo\n"
+   "#define bar()\\\n"
+   "if (A) { \\\n"
+   "if (B) { \\\n"
+   "C(); \\\n"
+   "}\\\n"
+   "}\\\n"
+   "D();\n"
+   "#endif\n"
+   "#endif",
+   style);
+  verifyFormat("#ifndef foo\n"
+   "#define foo\n"
+   "if (emacs) {\n"
+   "#ifdef is\n"
+   "#define lit   \\\n"
+   "if (af) { \\\n"
+   "return duh(); \\\n"
+   "}\n"
+   "#endif\n"
+   "}\n"
+   "#endif",
+   style);
+
+  style.PPIndentWidth = 2;
+  verifyFormat("#ifdef foo\n"
+   "#define bar() \\\n"
+   "if (A) {  \\\n"
+   "B();  \\\n"
+   "} \\\n"
+   "C();\n"
+   "#endif",
+   style);
+  style.IndentWidth = 8;
+  verifyFormat("#ifdef foo\n"
+   "#define bar()\\\n"
+   "if (A) { \\\n"
+   "B(); \\\n"
+   "}\\\n"
+   "C();\n"
+   "#endif",
+   style);
+
+  style.IndentWidth = 4;
+  style.PPIndentWidth = 1;
+
+  style.IndentPPDirectives = FormatStyle::PPDIS_AfterHash;
+  verifyFormat("#ifdef X\n"
+   "# define Y  \\\n"
+   " switch (Y) {   \\\n"
+   " case 0:\\\n"
+   " break; \\\n"
+   " case 1:\\\n"
+   " break; \\\n"
+   " }  \\\n"
+   " Z();\n"
+   "#endif",
+   style);
+  verifyFormat("#ifdef foo\n"
+   "# define bar() \\\n"
+   " if (A) {  \\\n"
+   " B();  \\\n"
+   " } \\\n"
+   " C();\n"
+   "#endif",
+   style);
+  verifyFormat("if (emacs) {\n"
+   "#ifdef is\n"
+   "# define lit   \\\n"
+   " if (af) { \\\n"
+   " return duh(); \\\n"
+   " }\n"
+   "#endif\n"
+   "}",
+   style);
+  verifyFormat("#if abc\n"
+   "# ifdef foo\n"
+   "#  define bar()\\\n"
+   "  if (A) { \\\n"
+   "  if (B) { \\\n"
+   "  C(); \\\n"
+   "  }\\\n"
+   "  }\\\n"
+   "  D();\n"
+   "# endif\

[PATCH] D137181: [clang-format] Don't use 'PPIndentWidth' inside multi-line macros

[Noah Goldstein via Phabricator via cfe-commits]

goldstein.w.n added a comment.

In D137181#3914245 , @owenpan wrote:

> In D137181#3914083 , @goldstein.w.n 
> wrote:
>
>> In D137181#3914066 , @owenpan 
>> wrote:
>>
>>> @goldstein.w.n do you need to modify MacroCallReconstructor.cpp, Macros.h, 
>>> and MacroCallReconstructorTest.cpp? Leaving them out wouldn't break any 
>>> existing tests.
>>
>> Hi, sorry to go mia all day. We can't drop `PPLevel` from 
>> `MacroCallReconstructor` because we need it for the `createUnwrappedLine` 
>> MacroCallReconstructor.cpp:L66.
>
> Why would you need it for `createUnwrappedLine`? Can you add test cases for 
> the requirement? Removing all the changes to `MacroCallReconstructor` doesn't 
> fail any existing and new tests.

Okay fair enough. Dropped.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137181

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

[PATCH] D137181: [clang-format] Don't use 'PPIndentWidth' inside multi-line macros

[Noah Goldstein via Phabricator via cfe-commits]

goldstein.w.n updated this revision to Diff 473906.
goldstein.w.n added a comment.



1. Updating D137181 : [clang-format] Don't 
use 'PPIndentWidth' inside multi-line macros #
2. Enter a brief description of the changes included in this update.
3. The first line is used as subject, next lines as comment. #
4. If you intended to create a new revision, use:
5. $ arc diff --create

Remove changes to MacroCallReconstructor


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137181

Files:
  clang/lib/Format/TokenAnnotator.h
  clang/lib/Format/UnwrappedLineFormatter.cpp
  clang/lib/Format/UnwrappedLineParser.cpp
  clang/lib/Format/UnwrappedLineParser.h
  clang/unittests/Format/FormatTest.cpp

Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -5040,6 +5040,189 @@
"  int y = 0;\n"
"}",
style);
+
+  style.IndentPPDirectives = FormatStyle::PPDIS_None;
+  verifyFormat("#ifdef X\n"
+   "#define Y   \\\n"
+   " switch (Y) {   \\\n"
+   " case 0:\\\n"
+   " break; \\\n"
+   " case 1:\\\n"
+   " break; \\\n"
+   " }  \\\n"
+   " Z();\n"
+   "#endif",
+   style);
+  verifyFormat("#ifdef foo\n"
+   "#define bar() \\\n"
+   " if (A) { \\\n"
+   " B(); \\\n"
+   " }\\\n"
+   " C();\n"
+   "#endif",
+   style);
+  verifyFormat("if (emacs) {\n"
+   "#ifdef is\n"
+   "#define lit\\\n"
+   " if (af) { \\\n"
+   " return duh(); \\\n"
+   " }\n"
+   "#endif\n"
+   "}",
+   style);
+  verifyFormat("#if abc\n"
+   "#ifdef foo\n"
+   "#define bar() \\\n"
+   " if (A) { \\\n"
+   " if (B) { \\\n"
+   " C(); \\\n"
+   " }\\\n"
+   " }\\\n"
+   " D();\n"
+   "#endif\n"
+   "#endif",
+   style);
+  verifyFormat("#ifndef foo\n"
+   "#define foo\n"
+   "if (emacs) {\n"
+   "#ifdef is\n"
+   "#define lit\\\n"
+   " if (af) { \\\n"
+   " return duh(); \\\n"
+   " }\n"
+   "#endif\n"
+   "}\n"
+   "#endif",
+   style);
+
+  style.IndentPPDirectives = FormatStyle::PPDIS_AfterHash;
+  verifyFormat("#ifdef X\n"
+   "# define Y   \\\n"
+   "  switch (Y) {   \\\n"
+   "  case 0:\\\n"
+   "  break; \\\n"
+   "  case 1:\\\n"
+   "  break; \\\n"
+   "  }  \\\n"
+   "  Z();\n"
+   "#endif",
+   style);
+  verifyFormat("#ifdef foo\n"
+   "# define bar() \\\n"
+   "  if (A) { \\\n"
+   "  B(); \\\n"
+   "  }\\\n"
+   "  C();\n"
+   "#endif",
+   style);
+  verifyFormat("if (emacs) {\n"
+   "#ifdef is\n"
+   "# define lit\\\n"
+   "  if (af) { \\\n"
+   "  return duh(); \\\n"
+   "  }\n"
+   "#endif\n"
+   "}",
+   style);
+  verifyFormat("#if abc\n"
+   "# ifdef foo\n"
+   "#  define bar() \\\n"
+   "   if (A) { \\\n"
+   "   if (B) { \\\n"
+   "   C(); \\\n"
+   "   }\\\n"
+   "   }\\\n"
+   "   D();\n"
+   "# endif\n"
+   "#endif",
+   style);
+  verifyFormat("#if abc\n"
+   "# ifdef foo\n"
+   "#  define bar() \\\n"
+   "   if (A) { \\\n"
+   "   B(); \\\n"
+   "   }\\\n"
+   "   C();\n"
+   "# endif\n"
+   "#endif",
+   style);
+  verifyFormat("#ifndef foo\n"
+   "#define foo\n"
+   "if (emacs) {\n"
+   "#ifdef is\n"
+   "# define lit\\\n"
+   "  if (af) { \\\n"
+   "  return duh(); \\\n"
+   "  }\n"
+   "#endif\n"
+   "}\n"
+   "#endif",
+   style);
+
+  style.IndentPPDirectives = FormatStyle::PPDIS_Befor

[PATCH] D137181: [clang-format] Don't use 'PPIndentWidth' inside multi-line macros

[Noah Goldstein via Phabricator via cfe-commits]

goldstein.w.n added a comment.

In D137181#3911602 , @owenpan wrote:

> In D137181#3911005 , @sstwcw wrote:
>
>> Here is one problem:
>>
>>   clang-format -style='{IndentPPDirectives: BeforeHash, PPIndentWidth: 1, 
>> IndentWidth: 4, IndentCaseLabels: true}'
>>   
>>   actual:
>>   #define X  
>> \
>>switch (x) {  
>> \
>>case 0:   
>> \
>> break;   
>> \
>>default:  
>> \
>> break;   
>> \
>>}
>>   
>>   expected:
>>   #define X  
>> \
>>switch (x) {  
>> \
>>case 0:   
>> \
>>break;
>> \
>>default:  
>> \
>>break;
>> \
>>}
>
> Actually, the expected should be:
>
>   #define X   
>   \
>   switch (x) {
>   \
>   case 0: 
>   \
>   break;  
>   \
>   default:
>   \
>   break;  
>   \
>   }

Err I'm not sure that's right. I thought we where going for the C-code should 
start at the column that the 'd' in define is.
So it would be:

  #define X   \
   switch (x) {\
   case 0:   \
   break;\
   default:  \
   break;\
   }


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137181

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

[PATCH] D137181: [clang-format] Don't use 'PPIndentWidth' inside multi-line macros

[Noah Goldstein via Phabricator via cfe-commits]

goldstein.w.n updated this revision to Diff 473875.
goldstein.w.n added a comment.



1. Updating D137181 : [clang-format] Don't 
use 'PPIndentWidth' inside multi-line macros #
2. Enter a brief description of the changes included in this update.
3. The first line is used as subject, next lines as comment. #
4. If you intended to create a new revision, use:
5. $ arc diff --create

Fixed case statement, remove many unnecessary PPLevel changes.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137181

Files:
  clang/lib/Format/MacroCallReconstructor.cpp
  clang/lib/Format/Macros.h
  clang/lib/Format/TokenAnnotator.h
  clang/lib/Format/UnwrappedLineFormatter.cpp
  clang/lib/Format/UnwrappedLineParser.cpp
  clang/lib/Format/UnwrappedLineParser.h
  clang/unittests/Format/FormatTest.cpp
  clang/unittests/Format/MacroCallReconstructorTest.cpp

Index: clang/unittests/Format/MacroCallReconstructorTest.cpp
===
--- clang/unittests/Format/MacroCallReconstructorTest.cpp
+++ clang/unittests/Format/MacroCallReconstructorTest.cpp
@@ -194,7 +194,7 @@
   Expansion Exp(Lex, *Macros);
   TokenList Call = Exp.expand("X");
 
-  MacroCallReconstructor Unexp(0, Exp.getUnexpanded());
+  MacroCallReconstructor Unexp(0, 0, Exp.getUnexpanded());
   Unexp.addLine(line(Exp.getTokens()));
   EXPECT_TRUE(Unexp.finished());
   Matcher U(Call, Lex);
@@ -206,7 +206,7 @@
   Expansion Exp(Lex, *Macros);
   TokenList Call = Exp.expand("C", {"void f()"});
 
-  MacroCallReconstructor Unexp(0, Exp.getUnexpanded());
+  MacroCallReconstructor Unexp(0, 0, Exp.getUnexpanded());
   Matcher E(Exp.getTokens(), Lex);
   Unexp.addLine(line(E.consume("class X {")));
   EXPECT_FALSE(Unexp.finished());
@@ -228,7 +228,7 @@
 
   UnexpandedMap Unexpanded =
   mergeUnexpanded(Exp1.getUnexpanded(), Exp2.getUnexpanded());
-  MacroCallReconstructor Unexp(0, Unexpanded);
+  MacroCallReconstructor Unexp(0, 0, Unexpanded);
   Matcher E(Exp2.getTokens(), Lex);
   Unexp.addLine(line(E.consume("a")));
   EXPECT_FALSE(Unexp.finished());
@@ -257,7 +257,7 @@
   TokenList Call2 = Exp.expand("SEMI");
   TokenList Call3 = Exp.expand("SEMI");
 
-  MacroCallReconstructor Unexp(0, Exp.getUnexpanded());
+  MacroCallReconstructor Unexp(0, 0, Exp.getUnexpanded());
   Matcher E(Exp.getTokens(), Lex);
   Unexp.addLine(line(E.consume(";")));
   EXPECT_TRUE(Unexp.finished());
@@ -296,7 +296,7 @@
   // }
   UnexpandedMap Unexpanded =
   mergeUnexpanded(Exp1.getUnexpanded(), Exp2.getUnexpanded());
-  MacroCallReconstructor Unexp(0, Unexpanded);
+  MacroCallReconstructor Unexp(0, 0, Unexpanded);
   Matcher E(Exp2.getTokens(), Lex);
   Unexp.addLine(line(E.consume("{")));
   EXPECT_FALSE(Unexp.finished());
@@ -357,7 +357,7 @@
   UnexpandedMap Unexpanded = mergeUnexpanded(
   Exp1.getUnexpanded(),
   mergeUnexpanded(Exp2.getUnexpanded(), Exp3.getUnexpanded()));
-  MacroCallReconstructor Unexp(0, Unexpanded);
+  MacroCallReconstructor Unexp(0, 0, Unexpanded);
   Matcher E(Exp3.getTokens(), Lex);
   Unexp.addLine(line(E.consume("{")));
   Unexp.addLine(
@@ -409,7 +409,7 @@
   Expansion Exp(Lex, *Macros);
   TokenList Call = Exp.expand("CALL", {std::string("int a"), "int b"});
 
-  MacroCallReconstructor Unexp(0, Exp.getUnexpanded());
+  MacroCallReconstructor Unexp(0, 0, Exp.getUnexpanded());
   Matcher E(Exp.getTokens(), Lex);
   Unexp.addLine(line({
   E.consume("f([] {"),
@@ -430,7 +430,7 @@
   Expansion Exp(Lex, *Macros);
   TokenList Call = Exp.expand("CALL", {std::string("x"), "y"});
 
-  MacroCallReconstructor Unexp(0, Exp.getUnexpanded());
+  MacroCallReconstructor Unexp(0, 0, Exp.getUnexpanded());
   Matcher E(Exp.getTokens(), Lex);
   Unexp.addLine(line(E.consume("y + x")));
   EXPECT_TRUE(Unexp.finished());
@@ -444,7 +444,7 @@
   Expansion Exp(Lex, *Macros);
   TokenList Call = Exp.expand("ID", {std::string("x; x"), "y"});
 
-  MacroCallReconstructor Unexp(0, Exp.getUnexpanded());
+  MacroCallReconstructor Unexp(0, 0, Exp.getUnexpanded());
   Matcher E(Exp.getTokens(), Lex);
   Unexp.addLine(line(E.consume("x;")));
   Unexp.addLine(line(E.consume("x y")));
@@ -482,7 +482,7 @@
   // 2: }
   UnexpandedMap Unexpanded =
   mergeUnexpanded(Exp1.getUnexpanded(), Exp2.getUnexpanded());
-  MacroCallReconstructor Unexp(0, Unexpanded);
+  MacroCallReconstructor Unexp(0, 0, Unexpanded);
   Matcher E(Exp2.getTokens(), Lex);
   Unexp.addLine(line(E.consume("{")));
   Unexp.addLine(line(E.consume("a * b;")));
@@ -524,7 +524,7 @@
 
   auto Prefix = tokens("int a = []() {");
   auto Postfix = tokens("}();");
-  MacroCallReconstructor Unexp(0, Exp.getUnexpanded());
+  MacroCallReconstructor Unexp(0, 0, Exp.getUnexpanded());
   Matcher E(Exp.getTokens(), Lex);
   Unexp.addLine(line({
   Prefix,
@@ -560,7 +560,7 @@
   Expansion Exp(Lex, *Macros);
   TokenList Call = Exp.expand("X", {"a", "b", "c"

[PATCH] D137181: [clang-format] Don't use 'PPIndentWidth' inside multi-line macros

[Noah Goldstein via Phabricator via cfe-commits]

goldstein.w.n added a comment.

In D137181#3914066 , @owenpan wrote:

> @goldstein.w.n do you need to modify MacroCallReconstructor.cpp, Macros.h, 
> and MacroCallReconstructorTest.cpp? Leaving them out wouldn't break any 
> existing tests.
>
> Adding `PPBranchLevel` (or `PPLevel` in your case) to `UnwrappedLine` and 
> `AnnotatedLine` worked for me. I suggest that you leave `PPLevel` alone in 
> the annotator and don't change it at various places in the parser.

Hi, sorry to go mia all day. You're right I can drop all uses in the 
`MacroCallReconstructor`. Also realized some of my placements where incorrect 
(that's what was breaking the case statement example). Will drop `PPLevel` in 
`MacroCallReconstructor` and add tests for case statement shortly.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137181

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

[PATCH] D137181: [clang-format] Don't use 'PPIndentWidth' inside multi-line macros

[Noah Goldstein via Phabricator via cfe-commits]

goldstein.w.n added a comment.

In D137181#3911602 , @owenpan wrote:

> In D137181#3911005 , @sstwcw wrote:
>
>> Here is one problem:
>>
>>   clang-format -style='{IndentPPDirectives: BeforeHash, PPIndentWidth: 1, 
>> IndentWidth: 4, IndentCaseLabels: true}'
>>   
>>   actual:
>>   #define X  
>> \
>>switch (x) {  
>> \
>>case 0:   
>> \
>> break;   
>> \
>>default:  
>> \
>> break;   
>> \
>>}
>>   
>>   expected:
>>   #define X  
>> \
>>switch (x) {  
>> \
>>case 0:   
>> \
>>break;
>> \
>>default:  
>> \
>>break;
>> \
>>}
>
> Actually, the expected should be:
>
>   #define X   
>   \
>   switch (x) {
>   \
>   case 0: 
>   \
>   break;  
>   \
>   default:
>   \
>   break;  
>   \
>   }

Was able to fix this, will post new code with a test for it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137181

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

[PATCH] D137181: [clang-format] Don't use 'PPIndentWidth' inside multi-line macros

[Noah Goldstein via Phabricator via cfe-commits]

goldstein.w.n added a comment.

In D137181#3910799 , @owenpan wrote:

> IMO we should find a simpler way to indent bodies of macro definitions that 
> are nested more than one level deep. I prefer we handle that in another patch 
> and only support the examples in the summary for now.

I'm not sure what changes to the patch that would imply?
I can drop the double-indented tests but seems to me that it would simply be 
buggy if it overintented the code due to header guards or if the define was 
placed in code.

Maybe would be cleaner if we made `Level` a new struct and overloading +/- to 
check if in PP region to keep track of PP level?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137181

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

[PATCH] D137181: [clang-format] Don't use 'PPIndentWidth' inside multi-line macros

[Noah Goldstein via Phabricator via cfe-commits]

goldstein.w.n added a comment.

In D137181#3910566 , @owenpan wrote:

> In D137181#3910404 , @goldstein.w.n 
> wrote:
>
>> Doesn't that add an arbitrary +1 to the begining of the indentation?
>> Shouldn't it be:
>>
>>   // IndentPPDirectives: AfterHash
>>   #ifdef foo
>>   # define bar() \\
>> if (A) { \\
>> B(); \\
>> }\\
>> C();
>>   #endif
>>   
>>   // IndentPPDirectives: BeforeHash
>>   #ifdef foo
>>#define bar() \\
>> if (A) { \\
>> B(); \\
>> }\\
>> C();
>>   #endif
>>   
>>   // IndentPPDirectives: NoneHash
>>   #ifdef foo
>>   #define bar() \\
>>if (A) { \\
>>B(); \\
>>}\\
>>C();
>>   #endif
>
> Given the settings used in your example:
>
>   PPIndentWidth: 1
>   IndentWidth: 4
>
> IMO the macro body should be shifted to the right by 1 column (except when 
> `IndentPPDirectives` is set to `None`). That is, the indent of the macro body 
> relative to the start of `define` should be the same with any setting of 
> `IndentPPDirective`.

Okay set it up so that where the 'd' in "define" is is always where indentation 
starts.
Essentially track `PPLevel` independently of `Level` and use the two seperarely.

I probably missed a few places / added in a few bad places.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137181

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

[PATCH] D137181: [clang-format] Don't use 'PPIndentWidth' inside multi-line macros

[Noah Goldstein via Phabricator via cfe-commits]

goldstein.w.n updated this revision to Diff 473489.
goldstein.w.n added a comment.



1. Updating D137181 : [clang-format] Don't 
use 'PPIndentWidth' inside multi-line macros #
2. Enter a brief description of the changes included in this update.
3. The first line is used as subject, next lines as comment. #
4. If you intended to create a new revision, use:
5. $ arc diff --create

Make it so that code indentation always starts where 'd' in "define" is.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137181

Files:
  clang/lib/Format/MacroCallReconstructor.cpp
  clang/lib/Format/Macros.h
  clang/lib/Format/TokenAnnotator.cpp
  clang/lib/Format/TokenAnnotator.h
  clang/lib/Format/UnwrappedLineFormatter.cpp
  clang/lib/Format/UnwrappedLineParser.cpp
  clang/lib/Format/UnwrappedLineParser.h
  clang/unittests/Format/FormatTest.cpp
  clang/unittests/Format/MacroCallReconstructorTest.cpp

Index: clang/unittests/Format/MacroCallReconstructorTest.cpp
===
--- clang/unittests/Format/MacroCallReconstructorTest.cpp
+++ clang/unittests/Format/MacroCallReconstructorTest.cpp
@@ -194,7 +194,7 @@
   Expansion Exp(Lex, *Macros);
   TokenList Call = Exp.expand("X");
 
-  MacroCallReconstructor Unexp(0, Exp.getUnexpanded());
+  MacroCallReconstructor Unexp(0, 0, Exp.getUnexpanded());
   Unexp.addLine(line(Exp.getTokens()));
   EXPECT_TRUE(Unexp.finished());
   Matcher U(Call, Lex);
@@ -206,7 +206,7 @@
   Expansion Exp(Lex, *Macros);
   TokenList Call = Exp.expand("C", {"void f()"});
 
-  MacroCallReconstructor Unexp(0, Exp.getUnexpanded());
+  MacroCallReconstructor Unexp(0, 0, Exp.getUnexpanded());
   Matcher E(Exp.getTokens(), Lex);
   Unexp.addLine(line(E.consume("class X {")));
   EXPECT_FALSE(Unexp.finished());
@@ -228,7 +228,7 @@
 
   UnexpandedMap Unexpanded =
   mergeUnexpanded(Exp1.getUnexpanded(), Exp2.getUnexpanded());
-  MacroCallReconstructor Unexp(0, Unexpanded);
+  MacroCallReconstructor Unexp(0, 0, Unexpanded);
   Matcher E(Exp2.getTokens(), Lex);
   Unexp.addLine(line(E.consume("a")));
   EXPECT_FALSE(Unexp.finished());
@@ -257,7 +257,7 @@
   TokenList Call2 = Exp.expand("SEMI");
   TokenList Call3 = Exp.expand("SEMI");
 
-  MacroCallReconstructor Unexp(0, Exp.getUnexpanded());
+  MacroCallReconstructor Unexp(0, 0, Exp.getUnexpanded());
   Matcher E(Exp.getTokens(), Lex);
   Unexp.addLine(line(E.consume(";")));
   EXPECT_TRUE(Unexp.finished());
@@ -296,7 +296,7 @@
   // }
   UnexpandedMap Unexpanded =
   mergeUnexpanded(Exp1.getUnexpanded(), Exp2.getUnexpanded());
-  MacroCallReconstructor Unexp(0, Unexpanded);
+  MacroCallReconstructor Unexp(0, 0, Unexpanded);
   Matcher E(Exp2.getTokens(), Lex);
   Unexp.addLine(line(E.consume("{")));
   EXPECT_FALSE(Unexp.finished());
@@ -357,7 +357,7 @@
   UnexpandedMap Unexpanded = mergeUnexpanded(
   Exp1.getUnexpanded(),
   mergeUnexpanded(Exp2.getUnexpanded(), Exp3.getUnexpanded()));
-  MacroCallReconstructor Unexp(0, Unexpanded);
+  MacroCallReconstructor Unexp(0, 0, Unexpanded);
   Matcher E(Exp3.getTokens(), Lex);
   Unexp.addLine(line(E.consume("{")));
   Unexp.addLine(
@@ -409,7 +409,7 @@
   Expansion Exp(Lex, *Macros);
   TokenList Call = Exp.expand("CALL", {std::string("int a"), "int b"});
 
-  MacroCallReconstructor Unexp(0, Exp.getUnexpanded());
+  MacroCallReconstructor Unexp(0, 0, Exp.getUnexpanded());
   Matcher E(Exp.getTokens(), Lex);
   Unexp.addLine(line({
   E.consume("f([] {"),
@@ -430,7 +430,7 @@
   Expansion Exp(Lex, *Macros);
   TokenList Call = Exp.expand("CALL", {std::string("x"), "y"});
 
-  MacroCallReconstructor Unexp(0, Exp.getUnexpanded());
+  MacroCallReconstructor Unexp(0, 0, Exp.getUnexpanded());
   Matcher E(Exp.getTokens(), Lex);
   Unexp.addLine(line(E.consume("y + x")));
   EXPECT_TRUE(Unexp.finished());
@@ -444,7 +444,7 @@
   Expansion Exp(Lex, *Macros);
   TokenList Call = Exp.expand("ID", {std::string("x; x"), "y"});
 
-  MacroCallReconstructor Unexp(0, Exp.getUnexpanded());
+  MacroCallReconstructor Unexp(0, 0, Exp.getUnexpanded());
   Matcher E(Exp.getTokens(), Lex);
   Unexp.addLine(line(E.consume("x;")));
   Unexp.addLine(line(E.consume("x y")));
@@ -482,7 +482,7 @@
   // 2: }
   UnexpandedMap Unexpanded =
   mergeUnexpanded(Exp1.getUnexpanded(), Exp2.getUnexpanded());
-  MacroCallReconstructor Unexp(0, Unexpanded);
+  MacroCallReconstructor Unexp(0, 0, Unexpanded);
   Matcher E(Exp2.getTokens(), Lex);
   Unexp.addLine(line(E.consume("{")));
   Unexp.addLine(line(E.consume("a * b;")));
@@ -524,7 +524,7 @@
 
   auto Prefix = tokens("int a = []() {");
   auto Postfix = tokens("}();");
-  MacroCallReconstructor Unexp(0, Exp.getUnexpanded());
+  MacroCallReconstructor Unexp(0, 0, Exp.getUnexpanded());
   Matcher E(Exp.getTokens(), Lex);
   Unexp.addLine(line({
   Prefix,
@@ -560,7 +560,7 @@
   Expansion Exp(Lex, *Macros);
  

[PATCH] D137181: [clang-format] Don't use 'PPIndentWidth' inside multi-line macros

[Noah Goldstein via Phabricator via cfe-commits]

goldstein.w.n added a comment.

In D137181#3904383 , @owenpan wrote:

> With `ColumnLimit: 16` and `IndentPPDirectives: BeforeHash`, the format 
> should be:
>
>   #ifdef foo
>#define bar() \
>if (A) {  \
>B();  \
>} \
>C();
>   #endif
>
> With `IndentPPDirectives: AfterHash`, it should look like:
>
>   #ifdef foo
>   # define bar() \
>if (A) {  \
>B();  \
>} \
>C();
>   #endif

Doesn't that add an arbitrary +1 to the begining of the indentation?
Shouldn't it be:

  // IndentPPDirectives: AfterHash
  #ifdef foo
  # define bar() \\
if (A) { \\
B(); \\
}\\
C();
  #endif
  
  // IndentPPDirectives: BeforeHash
  #ifdef foo
   #define bar() \\
if (A) { \\
B(); \\
}\\
C();
  #endif
  
  // IndentPPDirectives: NoneHash
  #ifdef foo
  #define bar() \\
   if (A) { \\
   B(); \\
   }\\
   C();
  #endif


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137181

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

[PATCH] D137181: [clang-format] Don't use 'PPIndentWidth' inside multi-line macros

[Noah Goldstein via Phabricator via cfe-commits]

goldstein.w.n added inline comments.



Comment at: clang/unittests/Format/FormatTest.cpp:5056-5059
+   "if (A) { \\\n"
+   "B(); \\\n"
+   "}\\\n"
+   "C();\n"

goldstein.w.n wrote:
> owenpan wrote:
> > goldstein.w.n wrote:
> > > owenpan wrote:
> > > > I just noticed that here and below you got an extra `IndentWidth` than 
> > > > in the summary, so the patch only works for `PPDIS_None`?
> > > To a degree.
> > > 
> > > `Level` is used by both scope depth and PP depth so nested PP directives 
> > > with before/after will essentially have `IndentWidth * (PPLevel + 
> > > ScopeLevel)` as net indentation.
> > > 
> > > AFAICT `UnwrappedLineParser.cpp::parsePPDefine` needs to something other 
> > > than `Line->Level` with `PBBranchLevel + 1`.
> > > 
> > > I started doing that but the diff got a bit bigger given that it needs to 
> > > get propegated between Wrapper/Unwrapped/Annotated (AFAICT).
> > > 
> > > Would you prefer its implemented like that?
> > See D137181#3904383
> Hmm? What is that suppose to link to?
Err, sorry I see now. Got it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137181

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

[PATCH] D137181: [clang-format] Don't use 'PPIndentWidth' inside multi-line macros

[Noah Goldstein via Phabricator via cfe-commits]

goldstein.w.n added inline comments.



Comment at: clang/unittests/Format/FormatTest.cpp:5056-5059
+   "if (A) { \\\n"
+   "B(); \\\n"
+   "}\\\n"
+   "C();\n"

owenpan wrote:
> goldstein.w.n wrote:
> > owenpan wrote:
> > > I just noticed that here and below you got an extra `IndentWidth` than in 
> > > the summary, so the patch only works for `PPDIS_None`?
> > To a degree.
> > 
> > `Level` is used by both scope depth and PP depth so nested PP directives 
> > with before/after will essentially have `IndentWidth * (PPLevel + 
> > ScopeLevel)` as net indentation.
> > 
> > AFAICT `UnwrappedLineParser.cpp::parsePPDefine` needs to something other 
> > than `Line->Level` with `PBBranchLevel + 1`.
> > 
> > I started doing that but the diff got a bit bigger given that it needs to 
> > get propegated between Wrapper/Unwrapped/Annotated (AFAICT).
> > 
> > Would you prefer its implemented like that?
> See D137181#3904383
Hmm? What is that suppose to link to?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137181

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

[PATCH] D137181: [clang-format] Don't use 'PPIndentWidth' inside multi-line macros

[Noah Goldstein via Phabricator via cfe-commits]

goldstein.w.n added inline comments.



Comment at: clang/unittests/Format/FormatTest.cpp:5056-5059
+   "if (A) { \\\n"
+   "B(); \\\n"
+   "}\\\n"
+   "C();\n"

owenpan wrote:
> I just noticed that here and below you got an extra `IndentWidth` than in the 
> summary, so the patch only works for `PPDIS_None`?
To a degree.

`Level` is used by both scope depth and PP depth so nested PP directives with 
before/after will essentially have `IndentWidth * (PPLevel + ScopeLevel)` as 
net indentation.

AFAICT `UnwrappedLineParser.cpp::parsePPDefine` needs to something other than 
`Line->Level` with `PBBranchLevel + 1`.

I started doing that but the diff got a bit bigger given that it needs to get 
propegated between Wrapper/Unwrapped/Annotated (AFAICT).

Would you prefer its implemented like that?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137181

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

[PATCH] D137181: [clang-format] Don't use 'PPIndentWidth' inside multi-line macros

[Noah Goldstein via Phabricator via cfe-commits]

goldstein.w.n marked an inline comment as done.
goldstein.w.n added a comment.

In D137181#3899874 , @MyDeveloperDay 
wrote:

> This needs a unit test in clang/unittest/Format

Fixed in V2 I think but not sure as the summary doesn't seem to have changed.




Comment at: clang/unittests/Format/FormatTest.cpp:5054-5056
+   "#ifdef foo\n"
+   "#define bar() if (A) { B(); } C();\n"
+   "#endif",

owenpan wrote:
> goldstein.w.n wrote:
> > owenpan wrote:
> > > You can delete them.
> > Them being the ifdef/endif? If so will do for V2.
> Them being lines 5054-5056. (If you mouse over the comment, the lines will be 
> highlighted.)
Got it. Dropped in V2.



Comment at: clang/unittests/Format/FormatTest.cpp:5066-5068
+   "#ifdef foo\n"
+   "#define bar() if (A) { B(); } C();\n"
+   "#endif",

owenpan wrote:
> goldstein.w.n wrote:
> > owenpan wrote:
> > > Delete them or change to multiline format like lines 5048-5052.
> > Delete the test of the ifndef/endif? Can do either for V2.
> Actually, delete lines 5066-5068. There are mainly two ways to use 
> `verifyFormat`: `verifyFormat(Expected, Input, ...)` and `verifyFormat(Input, 
> ...)`. Here we can use the latter, in which `Input` is also expected output.
Got it. Dropped in V2.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137181

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

[PATCH] D137181: [clang-format] Don't use 'PPIndentWidth' inside multi-line macros

[Noah Goldstein via Phabricator via cfe-commits]

goldstein.w.n updated this revision to Diff 472759.
goldstein.w.n added a comment.



1. Updating D137181 : [clang-format] Don't 
use 'PPIndentWidth' inside multi-line macros #
2. Enter a brief description of the changes included in this update.
3. The first line is used as subject, next lines as comment. #
4. If you intended to create a new revision, use:
5. $ arc diff --create

1. Fixed indentation in commit
2. Fixed format nits in tests
3. Remove initial code from tests


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137181

Files:
  clang/lib/Format/UnwrappedLineFormatter.cpp
  clang/unittests/Format/FormatTest.cpp


Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -5040,6 +5040,34 @@
"  int y = 0;\n"
"}",
style);
+
+  style.IndentPPDirectives = FormatStyle::PPDIS_None;
+  verifyFormat("#ifdef foo\n"
+   "#define bar() \\\n"
+   "if (A) {  \\\n"
+   "B();  \\\n"
+   "} \\\n"
+   "C();\n"
+   "#endif",
+   style);
+  style.IndentPPDirectives = FormatStyle::PPDIS_AfterHash;
+  verifyFormat("#ifdef foo\n"
+   "# define bar()   \\\n"
+   "if (A) { \\\n"
+   "B(); \\\n"
+   "}\\\n"
+   "C();\n"
+   "#endif",
+   style);
+  style.IndentPPDirectives = FormatStyle::PPDIS_BeforeHash;
+  verifyFormat("#ifdef foo\n"
+   " #define bar()   \\\n"
+   "if (A) { \\\n"
+   "B(); \\\n"
+   "}\\\n"
+   "C();\n"
+   "#endif",
+   style);
 }
 
 TEST_F(FormatTest, IndentsPPDirectiveInReducedSpace) {
Index: clang/lib/Format/UnwrappedLineFormatter.cpp
===
--- clang/lib/Format/UnwrappedLineFormatter.cpp
+++ clang/lib/Format/UnwrappedLineFormatter.cpp
@@ -60,7 +60,7 @@
 // Update the indent level cache size so that we can rely on it
 // having the right size in adjustToUnmodifiedline.
 skipLine(Line, /*UnknownIndent=*/true);
-if (Line.InPPDirective ||
+if ((Line.InPPDirective && !Line.InMacroBody) ||
 (Style.IndentPPDirectives == FormatStyle::PPDIS_BeforeHash &&
  Line.Type == LT_CommentAbovePPDirective)) {
   unsigned IndentWidth =


Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -5040,6 +5040,34 @@
"  int y = 0;\n"
"}",
style);
+
+  style.IndentPPDirectives = FormatStyle::PPDIS_None;
+  verifyFormat("#ifdef foo\n"
+   "#define bar() \\\n"
+   "if (A) {  \\\n"
+   "B();  \\\n"
+   "} \\\n"
+   "C();\n"
+   "#endif",
+   style);
+  style.IndentPPDirectives = FormatStyle::PPDIS_AfterHash;
+  verifyFormat("#ifdef foo\n"
+   "# define bar()   \\\n"
+   "if (A) { \\\n"
+   "B(); \\\n"
+   "}\\\n"
+   "C();\n"
+   "#endif",
+   style);
+  style.IndentPPDirectives = FormatStyle::PPDIS_BeforeHash;
+  verifyFormat("#ifdef foo\n"
+   " #define bar()   \\\n"
+   "if (A) { \\\n"
+   "B(); \\\n"
+   "}\\\n"
+   "C();\n"
+   "#endif",
+   style);
 }
 
 TEST_F(FormatTest, IndentsPPDirectiveInReducedSpace) {
Index: clang/lib/Format/UnwrappedLineFormatter.cpp
===
--- clang/lib/Format/UnwrappedLineFormatter.cpp
+++ clang/lib/Format/UnwrappedLineFormatter.cpp
@@ -60,7 +60,7 @@
 // Update the indent level cache size so that we can rely on it
 // having the right size in adjustToUnmodifiedline.
 skipLine(Line, /*UnknownIndent=*/true);
-if (Line.InPPDirective ||
+if ((Line.InPPDirective && !Line.InMacroBody) ||
 (Style.IndentPPDirectives == FormatStyle::PPDIS_BeforeHash &&
  Line.Type == LT_CommentAbovePPDirective)) {
   unsigned IndentWidth =
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D137181: [clang-format] Don't use 'PPIndentWidth' inside multi-line macros

[Noah Goldstein via Phabricator via cfe-commits]

goldstein.w.n added a comment.

In D137181#3903161 , @owenpan wrote:

> Can you fix the format of the summary?

I see the misindentation of the After/Before hash examples. Will fix those for 
V2.
Anything else?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137181

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

[PATCH] D137181: [clang-format] Don't use 'PPIndentWidth' inside multi-line macros

[Noah Goldstein via Phabricator via cfe-commits]

goldstein.w.n added inline comments.



Comment at: clang/unittests/Format/FormatTest.cpp:5024
style);
+
   verifyFormat("#if 1\n"

owenpan wrote:
> Don't add an empty line here.
Will drop for V2.



Comment at: clang/unittests/Format/FormatTest.cpp:5045
+
+  style.ColumnLimit = 40;
+  style.IndentPPDirectives = FormatStyle::PPDIS_None;

owenpan wrote:
> Do you need it?
No, copied along when I was looking for examples of unit-tests with trailing 
'\'. I'll drop for V2



Comment at: clang/unittests/Format/FormatTest.cpp:5047
+  style.IndentPPDirectives = FormatStyle::PPDIS_None;
+  verifyFormat("#ifdef foo\n"
+   "#define bar() \\\n"

owenpan wrote:
> Do you need to enclose the macro definition in `#ifdef`/`#endif`?
No, but figured since this change is related to PP indentation the tests should 
have it. You would prefer I make the test just:

```
  verifyFormat("#define bar() \\\n"
   "if (A) {  \\\n"
   "B();  \\\n"
   "} \\\n"
   "C();\n",
   "#define bar() if (A) { B(); } C();\n",
   style);
```

?



Comment at: clang/unittests/Format/FormatTest.cpp:5054-5056
+   "#ifdef foo\n"
+   "#define bar() if (A) { B(); } C();\n"
+   "#endif",

owenpan wrote:
> You can delete them.
Them being the ifdef/endif? If so will do for V2.



Comment at: clang/unittests/Format/FormatTest.cpp:5066-5068
+   "#ifdef foo\n"
+   "#define bar() if (A) { B(); } C();\n"
+   "#endif",

owenpan wrote:
> Delete them or change to multiline format like lines 5048-5052.
Delete the test of the ifndef/endif? Can do either for V2.



Comment at: clang/unittests/Format/FormatTest.cpp:5078-5080
+   "#ifdef foo\n"
+   "#define bar() if (A) { B(); } C();\n"
+   "#endif",

owenpan wrote:
> Same here.
Same question as above, otherwise will do for V2.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137181

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

[PATCH] D137181: [clang-format] Don't use 'PPIndentWidth' inside multi-line macros

[Noah Goldstein via Phabricator via cfe-commits]

goldstein.w.n updated this revision to Diff 472397.
goldstein.w.n added a comment.



1. Updating D137181 : [clang-format] Don't 
use 'PPIndentWidth' inside multi-line macros #
2. Enter a brief description of the changes included in this update.
3. The first line is used as subject, next lines as comment. #
4. If you intended to create a new revision, use:
5. $ arc diff --create

Added unit tests.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137181

Files:
  clang/lib/Format/UnwrappedLineFormatter.cpp
  clang/unittests/Format/FormatTest.cpp


Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -5021,6 +5021,7 @@
"int y = 0;\n"
"}\n",
style);
+
   verifyFormat("#if 1\n"
" // some comments\n"
" // another\n"
@@ -5040,6 +5041,44 @@
"  int y = 0;\n"
"}",
style);
+
+  style.ColumnLimit = 40;
+  style.IndentPPDirectives = FormatStyle::PPDIS_None;
+  verifyFormat("#ifdef foo\n"
+   "#define bar() \\\n"
+   "if (A) {  \\\n"
+   "B();  \\\n"
+   "} \\\n"
+   "C();\n"
+   "#endif",
+   "#ifdef foo\n"
+   "#define bar() if (A) { B(); } C();\n"
+   "#endif",
+   style);
+  style.IndentPPDirectives = FormatStyle::PPDIS_AfterHash;
+  verifyFormat("#ifdef foo\n"
+   "# define bar()   \\\n"
+   "if (A) { \\\n"
+   "B(); \\\n"
+   "}\\\n"
+   "C();\n"
+   "#endif",
+   "#ifdef foo\n"
+   "#define bar() if (A) { B(); } C();\n"
+   "#endif",
+   style);
+  style.IndentPPDirectives = FormatStyle::PPDIS_BeforeHash;
+  verifyFormat("#ifdef foo\n"
+   " #define bar()   \\\n"
+   "if (A) { \\\n"
+   "B(); \\\n"
+   "}\\\n"
+   "C();\n"
+   "#endif",
+   "#ifdef foo\n"
+   "#define bar() if (A) { B(); } C();\n"
+   "#endif",
+   style);
 }
 
 TEST_F(FormatTest, IndentsPPDirectiveInReducedSpace) {
Index: clang/lib/Format/UnwrappedLineFormatter.cpp
===
--- clang/lib/Format/UnwrappedLineFormatter.cpp
+++ clang/lib/Format/UnwrappedLineFormatter.cpp
@@ -60,7 +60,7 @@
 // Update the indent level cache size so that we can rely on it
 // having the right size in adjustToUnmodifiedline.
 skipLine(Line, /*UnknownIndent=*/true);
-if (Line.InPPDirective ||
+if ((Line.InPPDirective && !Line.InMacroBody) ||
 (Style.IndentPPDirectives == FormatStyle::PPDIS_BeforeHash &&
  Line.Type == LT_CommentAbovePPDirective)) {
   unsigned IndentWidth =


Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -5021,6 +5021,7 @@
"int y = 0;\n"
"}\n",
style);
+
   verifyFormat("#if 1\n"
" // some comments\n"
" // another\n"
@@ -5040,6 +5041,44 @@
"  int y = 0;\n"
"}",
style);
+
+  style.ColumnLimit = 40;
+  style.IndentPPDirectives = FormatStyle::PPDIS_None;
+  verifyFormat("#ifdef foo\n"
+   "#define bar() \\\n"
+   "if (A) {  \\\n"
+   "B();  \\\n"
+   "} \\\n"
+   "C();\n"
+   "#endif",
+   "#ifdef foo\n"
+   "#define bar() if (A) { B(); } C();\n"
+   "#endif",
+   style);
+  style.IndentPPDirectives = FormatStyle::PPDIS_AfterHash;
+  verifyFormat("#ifdef foo\n"
+   "# define bar()   \\\n"
+   "if (A) { \\\n"
+   "B(); \\\n"
+   "}\\\n"
+   "C();\n"
+   "#endif",
+   "#ifdef foo\n"
+   "#define bar() if (A) { B(); } C();\n"
+   "#endif",
+   style);
+  style.IndentPPDirectives = FormatStyle::PPDIS_BeforeHash;
+  verifyFormat("#ifdef foo\n"
+   " #define bar()   \\\n"
+   "if (A) { \\\n"
+   "B(); \\\n"
+   "}\\\n"
+   "C();\n"
+   "#endif",
+   "#ifdef foo\n"
+   "#define bar() 

[PATCH] D137181: [clang-format] Don't use 'PPIndentWidth' inside multi-line macros

[Noah Goldstein via Phabricator via cfe-commits]

goldstein.w.n created this revision.
Herald added a project: All.
goldstein.w.n requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

  I.e with:
  PPIndentWidth = 1
  IndentWidth = 4
  ```
  #ifdef foo
  #define bar() if (A) { B(); } C();
  #endif
  ```
  
  Assume ColumnLimit Here:|
  
  IndentPPDirectives: None
  ```
  #ifdef foo
  #define bar()   \
  if (A) {\
  B();\
  }   \
  C();\
  #endif
  ```
  
  As opposed to
  ```
  #ifdef foo
  #define bar()   \
if (A) {  \
 B(); \
} \
C();
  #endif
  ```
  
  IndentPPDirectives: AfterHash
  ```
  #ifdef foo
  # define bar()  \
  if (A) {\
  B();\
  }   \
  C();\
  #endif
  ```
  
  As opposed to
  ```
  #ifdef foo
  # define bar()  \
if (A) {  \
 B(); \
} \
C();
  #endif
  ```
  
  IndentPPDirectives: BeforeHash
  ```
  #ifdef foo
   #define bar()  \
  if (A) {\
  B();\
  }   \
  C();\
  #endif
  ```
  
  As opposed to
  ```
  #ifdef foo
   #define bar()  \
if (A) {  \
 B(); \
} \
C();
  #endif
  ```


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D137181

Files:
  clang/lib/Format/UnwrappedLineFormatter.cpp


Index: clang/lib/Format/UnwrappedLineFormatter.cpp
===
--- clang/lib/Format/UnwrappedLineFormatter.cpp
+++ clang/lib/Format/UnwrappedLineFormatter.cpp
@@ -60,7 +60,7 @@
 // Update the indent level cache size so that we can rely on it
 // having the right size in adjustToUnmodifiedline.
 skipLine(Line, /*UnknownIndent=*/true);
-if (Line.InPPDirective ||
+if ((Line.InPPDirective && !Line.InMacroBody) ||
 (Style.IndentPPDirectives == FormatStyle::PPDIS_BeforeHash &&
  Line.Type == LT_CommentAbovePPDirective)) {
   unsigned IndentWidth =


Index: clang/lib/Format/UnwrappedLineFormatter.cpp
===
--- clang/lib/Format/UnwrappedLineFormatter.cpp
+++ clang/lib/Format/UnwrappedLineFormatter.cpp
@@ -60,7 +60,7 @@
 // Update the indent level cache size so that we can rely on it
 // having the right size in adjustToUnmodifiedline.
 skipLine(Line, /*UnknownIndent=*/true);
-if (Line.InPPDirective ||
+if ((Line.InPPDirective && !Line.InMacroBody) ||
 (Style.IndentPPDirectives == FormatStyle::PPDIS_BeforeHash &&
  Line.Type == LT_CommentAbovePPDirective)) {
   unsigned IndentWidth =
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits