Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add clarifying parenthesis around non-trivial conditions in ternary expressions. #90391

Merged
merged 2 commits into from
May 4, 2024

Conversation

luolent
Copy link
Contributor

@luolent luolent commented Apr 28, 2024

Fixes #85868

Parenthesis are added as requested on ternary operators with non trivial conditions.
I used this precedence table for reference, to make sure we get the expected behavior on each change.

@luolent luolent requested review from JDevlieghere and a team as code owners April 28, 2024 10:24
Copy link

Thank you for submitting a Pull Request (PR) to the LLVM Project!

This PR will be automatically labeled and the relevant teams will be
notified.

If you wish to, you can add reviewers by using the "Reviewers" section on this page.

If this is not working for you, it is probably because you do not have write
permissions for the repository. In which case you can instead tag reviewers by
name in a comment by using @ followed by their GitHub username.

If you have received no comments on your PR for a week, you can request a review
by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate
is once a week. Please remember that you are asking for valuable time from other developers.

If you have further questions, they may be answered by the LLVM GitHub User Guide.

You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums.

@llvmbot llvmbot added clang Clang issues not falling into any other category compiler-rt libc++abi libc++abi C++ Runtime Library. Not libc++. lld lldb backend:AMDGPU backend:SystemZ backend:X86 clang:frontend Language frontend issues, e.g. anything involving "Sema" xray libc lld:ELF mlir mlir:tosa labels Apr 28, 2024
@llvmbot
Copy link
Collaborator

llvmbot commented Apr 28, 2024

@llvm/pr-subscribers-mlir-tosa
@llvm/pr-subscribers-libcxxabi
@llvm/pr-subscribers-libc
@llvm/pr-subscribers-lld
@llvm/pr-subscribers-backend-x86
@llvm/pr-subscribers-lld-elf

@llvm/pr-subscribers-clang

Author: None (luolent)

Changes

Hi,

parenthesis were added as requested on ternary operators with non trivial conditions.
I used this precedence table to make sure we get the expected behavior on each change.

There is also a discussion on the issue, about creating a pass in clang-tidy in order to catch these missing parenthesis automatically.
Let me know if the above is desired and I can take it over.

Thanks!


Patch is 21.90 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/90391.diff

18 Files Affected:

  • (modified) clang/lib/Basic/Targets/AMDGPU.cpp (+1-1)
  • (modified) compiler-rt/lib/xray/xray_utils.h (+1-1)
  • (modified) libc/src/__support/FPUtil/aarch64/FEnvImpl.h (+10-10)
  • (modified) libc/src/__support/FPUtil/aarch64/fenv_darwin_impl.h (+24-24)
  • (modified) libc/src/__support/FPUtil/arm/FEnvImpl.h (+20-20)
  • (modified) libc/src/__support/FPUtil/riscv/FEnvImpl.h (+10-10)
  • (modified) libc/src/__support/FPUtil/x86_64/FEnvImpl.h (+12-12)
  • (modified) libclc/generic/lib/math/log_base.h (+1-1)
  • (modified) libcxxabi/src/cxa_personality.cpp (+1-1)
  • (modified) lld/ELF/LinkerScript.cpp (+1-1)
  • (modified) lldb/tools/debugserver/source/MacOSX/MachException.cpp (+1-1)
  • (modified) llvm/lib/Target/AMDGPU/AsmParser/AMDGPUAsmParser.cpp (+2-2)
  • (modified) llvm/lib/Target/AMDGPU/Disassembler/AMDGPUDisassembler.cpp (+2-2)
  • (modified) llvm/lib/Target/AVR/AVRAsmPrinter.cpp (+1-1)
  • (modified) llvm/lib/Target/SystemZ/SystemZInstrInfo.cpp (+2-2)
  • (modified) llvm/lib/Target/X86/AsmParser/X86AsmParser.cpp (+1-1)
  • (modified) llvm/lib/Target/X86/MCTargetDesc/X86MCCodeEmitter.cpp (+1-1)
  • (modified) mlir/lib/Dialect/Tosa/Transforms/TosaDecomposeTransposeConv.cpp (+2-2)
diff --git a/clang/lib/Basic/Targets/AMDGPU.cpp b/clang/lib/Basic/Targets/AMDGPU.cpp
index 5742885df0461b..cc7be64656e5b2 100644
--- a/clang/lib/Basic/Targets/AMDGPU.cpp
+++ b/clang/lib/Basic/Targets/AMDGPU.cpp
@@ -232,7 +232,7 @@ AMDGPUTargetInfo::AMDGPUTargetInfo(const llvm::Triple &Triple,
 
   HasLegalHalfType = true;
   HasFloat16 = true;
-  WavefrontSize = GPUFeatures & llvm::AMDGPU::FEATURE_WAVE32 ? 32 : 64;
+  WavefrontSize = (GPUFeatures & llvm::AMDGPU::FEATURE_WAVE32) ? 32 : 64;
   AllowAMDGPUUnsafeFPAtomics = Opts.AllowAMDGPUUnsafeFPAtomics;
 
   // Set pointer width and alignment for the generic address space.
diff --git a/compiler-rt/lib/xray/xray_utils.h b/compiler-rt/lib/xray/xray_utils.h
index 333826168c0db2..5dc73d7fa8cdea 100644
--- a/compiler-rt/lib/xray/xray_utils.h
+++ b/compiler-rt/lib/xray/xray_utils.h
@@ -61,7 +61,7 @@ constexpr size_t gcd(size_t a, size_t b) {
 constexpr size_t lcm(size_t a, size_t b) { return a * b / gcd(a, b); }
 
 constexpr size_t nearest_boundary(size_t number, size_t multiple) {
-  return multiple * ((number / multiple) + (number % multiple ? 1 : 0));
+  return multiple * ((number / multiple) + ((number % multiple) ? 1 : 0));
 }
 
 constexpr size_t next_pow2_helper(size_t num, size_t acc) {
diff --git a/libc/src/__support/FPUtil/aarch64/FEnvImpl.h b/libc/src/__support/FPUtil/aarch64/FEnvImpl.h
index d1d92169475d15..cd8a5970edd65a 100644
--- a/libc/src/__support/FPUtil/aarch64/FEnvImpl.h
+++ b/libc/src/__support/FPUtil/aarch64/FEnvImpl.h
@@ -53,19 +53,19 @@ struct FEnv {
   static constexpr uint32_t ExceptionControlFlagsBitPosition = 8;
 
   LIBC_INLINE static uint32_t getStatusValueForExcept(int excepts) {
-    return (excepts & FE_INVALID ? INVALID : 0) |
-           (excepts & FE_DIVBYZERO ? DIVBYZERO : 0) |
-           (excepts & FE_OVERFLOW ? OVERFLOW : 0) |
-           (excepts & FE_UNDERFLOW ? UNDERFLOW : 0) |
-           (excepts & FE_INEXACT ? INEXACT : 0);
+    return ((excepts & FE_INVALID) ? INVALID : 0) |
+           ((excepts & FE_DIVBYZERO) ? DIVBYZERO : 0) |
+           ((excepts & FE_OVERFLOW) ? OVERFLOW : 0) |
+           ((excepts & FE_UNDERFLOW) ? UNDERFLOW : 0) |
+           ((excepts & FE_INEXACT) ? INEXACT : 0);
   }
 
   LIBC_INLINE static int exceptionStatusToMacro(uint32_t status) {
-    return (status & INVALID ? FE_INVALID : 0) |
-           (status & DIVBYZERO ? FE_DIVBYZERO : 0) |
-           (status & OVERFLOW ? FE_OVERFLOW : 0) |
-           (status & UNDERFLOW ? FE_UNDERFLOW : 0) |
-           (status & INEXACT ? FE_INEXACT : 0);
+    return ((status & INVALID) ? FE_INVALID : 0) |
+           ((status & DIVBYZERO) ? FE_DIVBYZERO : 0) |
+           ((status & OVERFLOW) ? FE_OVERFLOW : 0) |
+           ((status & UNDERFLOW) ? FE_UNDERFLOW : 0) |
+           ((status & INEXACT) ? FE_INEXACT : 0);
   }
 
   static uint32_t getControlWord() {
diff --git a/libc/src/__support/FPUtil/aarch64/fenv_darwin_impl.h b/libc/src/__support/FPUtil/aarch64/fenv_darwin_impl.h
index 5b59ba38d67bb6..feb48e3719bf16 100644
--- a/libc/src/__support/FPUtil/aarch64/fenv_darwin_impl.h
+++ b/libc/src/__support/FPUtil/aarch64/fenv_darwin_impl.h
@@ -63,39 +63,39 @@ struct FEnv {
   // located in a different place from FE_FLUSHTOZERO status bit relative to
   // the other exceptions.
   LIBC_INLINE static uint32_t exception_value_from_status(int status) {
-    return (status & FE_INVALID ? EX_INVALID : 0) |
-           (status & FE_DIVBYZERO ? EX_DIVBYZERO : 0) |
-           (status & FE_OVERFLOW ? EX_OVERFLOW : 0) |
-           (status & FE_UNDERFLOW ? EX_UNDERFLOW : 0) |
-           (status & FE_INEXACT ? EX_INEXACT : 0) |
-           (status & FE_FLUSHTOZERO ? EX_FLUSHTOZERO : 0);
+    return ((status & FE_INVALID) ? EX_INVALID : 0) |
+           ((status & FE_DIVBYZERO) ? EX_DIVBYZERO : 0) |
+           ((status & FE_OVERFLOW) ? EX_OVERFLOW : 0) |
+           ((status & FE_UNDERFLOW) ? EX_UNDERFLOW : 0) |
+           ((status & FE_INEXACT) ? EX_INEXACT : 0) |
+           ((status & FE_FLUSHTOZERO) ? EX_FLUSHTOZERO : 0);
   }
 
   LIBC_INLINE static uint32_t exception_value_from_control(int control) {
-    return (control & __fpcr_trap_invalid ? EX_INVALID : 0) |
-           (control & __fpcr_trap_divbyzero ? EX_DIVBYZERO : 0) |
-           (control & __fpcr_trap_overflow ? EX_OVERFLOW : 0) |
-           (control & __fpcr_trap_underflow ? EX_UNDERFLOW : 0) |
-           (control & __fpcr_trap_inexact ? EX_INEXACT : 0) |
-           (control & __fpcr_flush_to_zero ? EX_FLUSHTOZERO : 0);
+    return ((control & __fpcr_trap_invalid) ? EX_INVALID : 0) |
+           ((control & __fpcr_trap_divbyzero) ? EX_DIVBYZERO : 0) |
+           ((control & __fpcr_trap_overflow) ? EX_OVERFLOW : 0) |
+           ((control & __fpcr_trap_underflow) ? EX_UNDERFLOW : 0) |
+           ((control & __fpcr_trap_inexact) ? EX_INEXACT : 0) |
+           ((control & __fpcr_flush_to_zero) ? EX_FLUSHTOZERO : 0);
   }
 
   LIBC_INLINE static int exception_value_to_status(uint32_t excepts) {
-    return (excepts & EX_INVALID ? FE_INVALID : 0) |
-           (excepts & EX_DIVBYZERO ? FE_DIVBYZERO : 0) |
-           (excepts & EX_OVERFLOW ? FE_OVERFLOW : 0) |
-           (excepts & EX_UNDERFLOW ? FE_UNDERFLOW : 0) |
-           (excepts & EX_INEXACT ? FE_INEXACT : 0) |
-           (excepts & EX_FLUSHTOZERO ? FE_FLUSHTOZERO : 0);
+    return ((excepts & EX_INVALID) ? FE_INVALID : 0) |
+           ((excepts & EX_DIVBYZERO) ? FE_DIVBYZERO : 0) |
+           ((excepts & EX_OVERFLOW) ? FE_OVERFLOW : 0) |
+           ((excepts & EX_UNDERFLOW) ? FE_UNDERFLOW : 0) |
+           ((excepts & EX_INEXACT) ? FE_INEXACT : 0) |
+           ((excepts & EX_FLUSHTOZERO) ? FE_FLUSHTOZERO : 0);
   }
 
   LIBC_INLINE static int exception_value_to_control(uint32_t excepts) {
-    return (excepts & EX_INVALID ? __fpcr_trap_invalid : 0) |
-           (excepts & EX_DIVBYZERO ? __fpcr_trap_divbyzero : 0) |
-           (excepts & EX_OVERFLOW ? __fpcr_trap_overflow : 0) |
-           (excepts & EX_UNDERFLOW ? __fpcr_trap_underflow : 0) |
-           (excepts & EX_INEXACT ? __fpcr_trap_inexact : 0) |
-           (excepts & EX_FLUSHTOZERO ? __fpcr_flush_to_zero : 0);
+    return ((excepts & EX_INVALID) ? __fpcr_trap_invalid : 0) |
+           ((excepts & EX_DIVBYZERO) ? __fpcr_trap_divbyzero : 0) |
+           ((excepts & EX_OVERFLOW) ? __fpcr_trap_overflow : 0) |
+           ((excepts & EX_UNDERFLOW) ? __fpcr_trap_underflow : 0) |
+           ((excepts & EX_INEXACT) ? __fpcr_trap_inexact : 0) |
+           ((excepts & EX_FLUSHTOZERO) ? __fpcr_flush_to_zero : 0);
   }
 
   LIBC_INLINE static uint32_t get_control_word() { return __arm_rsr("fpcr"); }
diff --git a/libc/src/__support/FPUtil/arm/FEnvImpl.h b/libc/src/__support/FPUtil/arm/FEnvImpl.h
index 78fbda4f7afff1..cb8d31d683af39 100644
--- a/libc/src/__support/FPUtil/arm/FEnvImpl.h
+++ b/libc/src/__support/FPUtil/arm/FEnvImpl.h
@@ -50,35 +50,35 @@ struct FEnv {
   }
 
   LIBC_INLINE static int exception_enable_bits_to_macro(uint32_t status) {
-    return (status & INVALID_ENABLE ? FE_INVALID : 0) |
-           (status & DIVBYZERO_ENABLE ? FE_DIVBYZERO : 0) |
-           (status & OVERFLOW_ENABLE ? FE_OVERFLOW : 0) |
-           (status & UNDERFLOW_ENABLE ? FE_UNDERFLOW : 0) |
-           (status & INEXACT_ENABLE ? FE_INEXACT : 0);
+    return ((status & INVALID_ENABLE) ? FE_INVALID : 0) |
+           ((status & DIVBYZERO_ENABLE) ? FE_DIVBYZERO : 0) |
+           ((status & OVERFLOW_ENABLE) ? FE_OVERFLOW : 0) |
+           ((status & UNDERFLOW_ENABLE) ? FE_UNDERFLOW : 0) |
+           ((status & INEXACT_ENABLE) ? FE_INEXACT : 0);
   }
 
   LIBC_INLINE static uint32_t exception_macro_to_enable_bits(int except) {
-    return (except & FE_INVALID ? INVALID_ENABLE : 0) |
-           (except & FE_DIVBYZERO ? DIVBYZERO_ENABLE : 0) |
-           (except & FE_OVERFLOW ? OVERFLOW_ENABLE : 0) |
-           (except & FE_UNDERFLOW ? UNDERFLOW_ENABLE : 0) |
-           (except & FE_INEXACT ? INEXACT_ENABLE : 0);
+    return ((except & FE_INVALID) ? INVALID_ENABLE : 0) |
+           ((except & FE_DIVBYZERO) ? DIVBYZERO_ENABLE : 0) |
+           ((except & FE_OVERFLOW) ? OVERFLOW_ENABLE : 0) |
+           ((except & FE_UNDERFLOW) ? UNDERFLOW_ENABLE : 0) |
+           ((except & FE_INEXACT) ? INEXACT_ENABLE : 0);
   }
 
   LIBC_INLINE static uint32_t exception_macro_to_status_bits(int except) {
-    return (except & FE_INVALID ? INVALID_STATUS : 0) |
-           (except & FE_DIVBYZERO ? DIVBYZERO_STATUS : 0) |
-           (except & FE_OVERFLOW ? OVERFLOW_STATUS : 0) |
-           (except & FE_UNDERFLOW ? UNDERFLOW_STATUS : 0) |
-           (except & FE_INEXACT ? INEXACT_STATUS : 0);
+    return ((except & FE_INVALID) ? INVALID_STATUS : 0) |
+           ((except & FE_DIVBYZERO) ? DIVBYZERO_STATUS : 0) |
+           ((except & FE_OVERFLOW) ? OVERFLOW_STATUS : 0) |
+           ((except & FE_UNDERFLOW) ? UNDERFLOW_STATUS : 0) |
+           ((except & FE_INEXACT) ? INEXACT_STATUS : 0);
   }
 
   LIBC_INLINE static uint32_t exception_status_bits_to_macro(int status) {
-    return (status & INVALID_STATUS ? FE_INVALID : 0) |
-           (status & DIVBYZERO_STATUS ? FE_DIVBYZERO : 0) |
-           (status & OVERFLOW_STATUS ? FE_OVERFLOW : 0) |
-           (status & UNDERFLOW_STATUS ? FE_UNDERFLOW : 0) |
-           (status & INEXACT_STATUS ? FE_INEXACT : 0);
+    return ((status & INVALID_STATUS) ? FE_INVALID : 0) |
+           ((status & DIVBYZERO_STATUS) ? FE_DIVBYZERO : 0) |
+           ((status & OVERFLOW_STATUS) ? FE_OVERFLOW : 0) |
+           ((status & UNDERFLOW_STATUS) ? FE_UNDERFLOW : 0) |
+           ((status & INEXACT_STATUS) ? FE_INEXACT : 0);
   }
 };
 
diff --git a/libc/src/__support/FPUtil/riscv/FEnvImpl.h b/libc/src/__support/FPUtil/riscv/FEnvImpl.h
index e7aee3ba4b9109..1de464a89de482 100644
--- a/libc/src/__support/FPUtil/riscv/FEnvImpl.h
+++ b/libc/src/__support/FPUtil/riscv/FEnvImpl.h
@@ -65,19 +65,19 @@ struct FEnv {
   }
 
   LIBC_INLINE static int exception_bits_to_macro(uint32_t status) {
-    return (status & INVALID ? FE_INVALID : 0) |
-           (status & DIVBYZERO ? FE_DIVBYZERO : 0) |
-           (status & OVERFLOW ? FE_OVERFLOW : 0) |
-           (status & UNDERFLOW ? FE_UNDERFLOW : 0) |
-           (status & INEXACT ? FE_INEXACT : 0);
+    return ((status & INVALID) ? FE_INVALID : 0) |
+           ((status & DIVBYZERO) ? FE_DIVBYZERO : 0) |
+           ((status & OVERFLOW) ? FE_OVERFLOW : 0) |
+           ((status & UNDERFLOW) ? FE_UNDERFLOW : 0) |
+           ((status & INEXACT) ? FE_INEXACT : 0);
   }
 
   LIBC_INLINE static uint32_t exception_macro_to_bits(int except) {
-    return (except & FE_INVALID ? INVALID : 0) |
-           (except & FE_DIVBYZERO ? DIVBYZERO : 0) |
-           (except & FE_OVERFLOW ? OVERFLOW : 0) |
-           (except & FE_UNDERFLOW ? UNDERFLOW : 0) |
-           (except & FE_INEXACT ? INEXACT : 0);
+    return ((except & FE_INVALID) ? INVALID : 0) |
+           ((except & FE_DIVBYZERO) ? DIVBYZERO : 0) |
+           ((except & FE_OVERFLOW) ? OVERFLOW : 0) |
+           ((except & FE_UNDERFLOW) ? UNDERFLOW : 0) |
+           ((except & FE_INEXACT) ? INEXACT : 0);
   }
 };
 
diff --git a/libc/src/__support/FPUtil/x86_64/FEnvImpl.h b/libc/src/__support/FPUtil/x86_64/FEnvImpl.h
index 0595658d7df328..a157b81aaaf325 100644
--- a/libc/src/__support/FPUtil/x86_64/FEnvImpl.h
+++ b/libc/src/__support/FPUtil/x86_64/FEnvImpl.h
@@ -72,25 +72,25 @@ static constexpr uint16_t MXCSR_EXCEPTION_CONTOL_BIT_POISTION = 7;
 LIBC_INLINE uint16_t get_status_value_for_except(int excepts) {
   // We will make use of the fact that exception control bits are single
   // bit flags in the control registers.
-  return (excepts & FE_INVALID ? ExceptionFlags::INVALID_F : 0) |
+  return ((excepts & FE_INVALID) ? ExceptionFlags::INVALID_F : 0) |
 #ifdef __FE_DENORM
-         (excepts & __FE_DENORM ? ExceptionFlags::DENORMAL_F : 0) |
+         ((excepts & __FE_DENORM) ? ExceptionFlags::DENORMAL_F : 0) |
 #endif // __FE_DENORM
-         (excepts & FE_DIVBYZERO ? ExceptionFlags::DIV_BY_ZERO_F : 0) |
-         (excepts & FE_OVERFLOW ? ExceptionFlags::OVERFLOW_F : 0) |
-         (excepts & FE_UNDERFLOW ? ExceptionFlags::UNDERFLOW_F : 0) |
-         (excepts & FE_INEXACT ? ExceptionFlags::INEXACT_F : 0);
+         ((excepts & FE_DIVBYZERO) ? ExceptionFlags::DIV_BY_ZERO_F : 0) |
+         ((excepts & FE_OVERFLOW) ? ExceptionFlags::OVERFLOW_F : 0) |
+         ((excepts & FE_UNDERFLOW) ? ExceptionFlags::UNDERFLOW_F : 0) |
+         ((excepts & FE_INEXACT) ? ExceptionFlags::INEXACT_F : 0);
 }
 
 LIBC_INLINE int exception_status_to_macro(uint16_t status) {
-  return (status & ExceptionFlags::INVALID_F ? FE_INVALID : 0) |
+  return ((status & ExceptionFlags::INVALID_F) ? FE_INVALID : 0) |
 #ifdef __FE_DENORM
-         (status & ExceptionFlags::DENORMAL_F ? __FE_DENORM : 0) |
+         ((status & ExceptionFlags::DENORMAL_F) ? __FE_DENORM : 0) |
 #endif // __FE_DENORM
-         (status & ExceptionFlags::DIV_BY_ZERO_F ? FE_DIVBYZERO : 0) |
-         (status & ExceptionFlags::OVERFLOW_F ? FE_OVERFLOW : 0) |
-         (status & ExceptionFlags::UNDERFLOW_F ? FE_UNDERFLOW : 0) |
-         (status & ExceptionFlags::INEXACT_F ? FE_INEXACT : 0);
+         ((status & ExceptionFlags::DIV_BY_ZERO_F) ? FE_DIVBYZERO : 0) |
+         ((status & ExceptionFlags::OVERFLOW_F) ? FE_OVERFLOW : 0) |
+         ((status & ExceptionFlags::UNDERFLOW_F) ? FE_UNDERFLOW : 0) |
+         ((status & ExceptionFlags::INEXACT_F) ? FE_INEXACT : 0);
 }
 
 struct X87StateDescriptor {
diff --git a/libclc/generic/lib/math/log_base.h b/libclc/generic/lib/math/log_base.h
index f5b6f1cb44991a..2558f016f60bef 100644
--- a/libclc/generic/lib/math/log_base.h
+++ b/libclc/generic/lib/math/log_base.h
@@ -289,7 +289,7 @@ log(double x)
     double ret = is_near ? ret_near : ret_far;
 
     ret = isinf(x) ? as_double(PINFBITPATT_DP64) : ret;
-    ret = isnan(x) | (x < 0.0) ? as_double(QNANBITPATT_DP64) : ret;
+    ret = (isnan(x) | (x < 0.0)) ? as_double(QNANBITPATT_DP64) : ret;
     ret = x == 0.0 ? as_double(NINFBITPATT_DP64) : ret;
     return ret;
 }
diff --git a/libcxxabi/src/cxa_personality.cpp b/libcxxabi/src/cxa_personality.cpp
index 4b6c4edbc26698..8f7914a8c6d128 100644
--- a/libcxxabi/src/cxa_personality.cpp
+++ b/libcxxabi/src/cxa_personality.cpp
@@ -717,7 +717,7 @@ static void scan_eh_tab(scan_results &results, _Unwind_Action actions,
             if (actionEntry == 0)
             {
                 // Found a cleanup
-                results.reason = actions & _UA_SEARCH_PHASE
+                results.reason = (actions & _UA_SEARCH_PHASE)
                                      ? _URC_CONTINUE_UNWIND
                                      : _URC_HANDLER_FOUND;
                 return;
diff --git a/lld/ELF/LinkerScript.cpp b/lld/ELF/LinkerScript.cpp
index f815b3ac6feeda..f9d8dcc4f71d95 100644
--- a/lld/ELF/LinkerScript.cpp
+++ b/lld/ELF/LinkerScript.cpp
@@ -801,7 +801,7 @@ static OutputDesc *addInputSec(StringMap<TinyPtrVector<OutputSection *>> &map,
       auto *firstIsec = cast<InputSectionBase>(
           cast<InputSectionDescription>(sec->commands[0])->sectionBases[0]);
       OutputSection *firstIsecOut =
-          firstIsec->flags & SHF_LINK_ORDER
+          (firstIsec->flags & SHF_LINK_ORDER)
               ? firstIsec->getLinkOrderDep()->getOutputSection()
               : nullptr;
       if (firstIsecOut != isec->getLinkOrderDep()->getOutputSection())
diff --git a/lldb/tools/debugserver/source/MacOSX/MachException.cpp b/lldb/tools/debugserver/source/MacOSX/MachException.cpp
index eab4cdfc8b775d..659fb2ff8186df 100644
--- a/lldb/tools/debugserver/source/MacOSX/MachException.cpp
+++ b/lldb/tools/debugserver/source/MacOSX/MachException.cpp
@@ -247,7 +247,7 @@ kern_return_t MachException::Message::Receive(mach_port_t port,
   DNBError err;
   const bool log_exceptions = DNBLogCheckLogBit(LOG_EXCEPTIONS);
   mach_msg_timeout_t mach_msg_timeout =
-      options & MACH_RCV_TIMEOUT ? timeout : 0;
+      (options & MACH_RCV_TIMEOUT) ? timeout : 0;
   if (log_exceptions && ((options & MACH_RCV_TIMEOUT) == 0)) {
     // Dump this log message if we have no timeout in case it never returns
     DNBLogThreaded("::mach_msg ( msg->{bits = %#x, size = %u remote_port = "
diff --git a/llvm/lib/Target/AMDGPU/AsmParser/AMDGPUAsmParser.cpp b/llvm/lib/Target/AMDGPU/AsmParser/AMDGPUAsmParser.cpp
index c4ec7a7befd47e..bd9c6a840bd3eb 100644
--- a/llvm/lib/Target/AMDGPU/AsmParser/AMDGPUAsmParser.cpp
+++ b/llvm/lib/Target/AMDGPU/AsmParser/AMDGPUAsmParser.cpp
@@ -3926,8 +3926,8 @@ bool AMDGPUAsmParser::validateMIMGAddrSize(const MCInst &Inst,
   const AMDGPU::MIMGBaseOpcodeInfo *BaseOpcode =
       AMDGPU::getMIMGBaseOpcodeInfo(Info->BaseOpcode);
   int VAddr0Idx = AMDGPU::getNamedOperandIdx(Opc, AMDGPU::OpName::vaddr0);
-  int RSrcOpName = Desc.TSFlags & SIInstrFlags::MIMG ? AMDGPU::OpName::srsrc
-                                                     : AMDGPU::OpName::rsrc;
+  int RSrcOpName = (Desc.TSFlags & SIInstrFlags::MIMG) ? AMDGPU::OpName::srsrc
+                                                       : AMDGPU::OpName::rsrc;
   int SrsrcIdx = AMDGPU::getNamedOperandIdx(Opc, RSrcOpName);
   int DimIdx = AMDGPU::getNamedOperandIdx(Opc, AMDGPU::OpName::dim);
   int A16Idx = AMDGPU::getNamedOperandIdx(Opc, AMDGPU::OpName::a16);
diff --git a/llvm/lib/Target/AMDGPU/Disassembler/AMDGPUDisassembler.cpp b/llvm/lib/Target/AMDGPU/Disassembler/AMDGPUDisassembler.cpp
index 8fd36b84a00cd8..05063c6c321a6a 100644
--- a/llvm/lib/Target/AMDGPU/Disassembler/AMDGPUDisassembler.cpp
+++ b/llvm/lib/Target/AMDGPU/Disassembler/AMDGPUDisassembler.cpp
@@ -921,8 +921,8 @@ void AMDGPUDisassembler::convertMIMGInst(MCInst &MI) const {
                                             AMDGPU::OpName::vdata);
   int VAddr0Idx =
       AMDGPU::getNamedOperandIdx(MI.getOpcode(), AMDGPU::OpName::vaddr0);
-  int RsrcOpName = TSFlags & SIInstrFlags::MIMG ? AMDGPU::OpName::srsrc
-                                                : AMDGPU::OpName::rsrc;
+  int RsrcOpName = (TSFlags & SIInstrFlags::MIMG) ? AMDGPU::OpName::srsrc
+                                                  : AMDGPU::OpName::rsrc;
   int RsrcIdx = AMDGPU::getNamedOperandIdx(MI.getOpcode(), RsrcOpName);
   int DMaskIdx = AMDGPU::getNamedOperandIdx(MI.getOpcode(),
                                             AMDGPU::OpName::dmask);
diff --git a/llvm/lib/Target/AVR/AVRAsmPrinter.cpp b/llvm/lib/Target/AVR/AVRAsmPrinter.cpp
index 1c8213b668f71a..0c7df65a428802 100644
--- a/llvm/lib/Target/AVR/AVRAsmPrinter.cpp
+++ b/llvm/lib/Target/AVR/AVRAsmPrinter.cpp
@@ -135,7 +135,7 @@ bool AVRAsmPrinter::PrintAsmOperand(const MachineInstr *MI, unsigned OpNum,
 
     if (BytesPerReg == 2) {
       Reg = TRI.getSubReg(Reg,
-                          ByteNumber % BytesPerReg ? AVR::sub_hi : AVR::sub_lo);
+                          (ByteNumber % BytesPerReg) ? AVR::sub_hi : AVR::sub_lo);
     }
 
     O << AVRInstPrinter::getPrettyRegisterName(Reg, MRI);
diff --git a/llvm/lib/Target/SystemZ/SystemZInstrInfo.cpp b/llvm/lib/Target/SystemZ/SystemZInstrInfo.cpp
index 6b75c30943b40a..98c854a62a2043 100644
--- a/llvm/lib/Target/SystemZ/SystemZInstrInfo.cpp
+++ b/llvm/lib/Target/SystemZ/SystemZInstrInfo.cpp
@@ -1897,8 +1897,8 @@ prepareCompareSwapOperands(MachineBasicBlock::iterator const MBBI) const {
 
 unsigned SystemZ::reverseCCMask(unsigned CCMask) {
   return ((CCMask & SystemZ::CCMASK_CMP_EQ) |
-          (CCMask & SystemZ::CCMASK_CMP_GT ? SystemZ::CCMASK_CMP_LT : 0) |
-          (CCMask & SystemZ::CCMASK_CMP_LT ? SystemZ::CCMASK_CMP_GT : 0) |
+          ((CCMask & SystemZ::CCMASK_CMP_GT) ? SystemZ::CCMASK_CMP_LT : 0) |
+          ((CCMask & SystemZ::CCMASK_CMP_LT) ? SystemZ::CCMASK_CMP_GT : 0) |
           (CCMask & SystemZ::CCMASK_CMP_UO));
 }
 
diff --git a/llvm/lib/Target/X86/AsmParser/X86AsmParser.cpp b/llvm/lib/Target/X86/AsmParser/X86AsmParser.cpp
index b05a036fb2f06b..2c2dc21f191d7a 100644
--- a/llvm/lib/Target/X86/AsmParser/X86AsmParser.cpp
+++ b/llvm/lib/Target/X86/AsmParser/X86AsmParser.cpp
@@ -3802,7 +3802,7 @@ bool X86AsmParser::validateInstruction(MCInst &Inst, const OperandVector &Ops) {
     //    VFMULCPHZrr   Dest, Src1, Src2
     //    VFMULCPHZrrk  Dest, Dest, Mask, Src1, Src2
     //    VFMULCPHZrrkz Dest, Mask, Src1, Src2
-    for (unsigned i = TSFlags & X86II::EVEX_K ? 2 : 1;
+    for (unsigned i = ((TSFlags & X86II::EVEX_K) ? 2 : 1);
          i < Inst.getNumOperands(); i++)
       i...
[truncated]

Copy link
Collaborator

@RKSimon RKSimon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please update the PR subject as its a lot more than just X86AsmParser.cpp

Copy link

github-actions bot commented Apr 28, 2024

✅ With the latest revision this PR passed the C/C++ code formatter.

Copy link
Collaborator

@RKSimon RKSimon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please address the clang-format warnings the CI has reported

@luolent
Copy link
Contributor Author

luolent commented Apr 28, 2024

Please update the PR subject as its a lot more than just X86AsmParser.cpp

Hi @RKSimon

All the issues mentioned are fixed. The title of the PR is misleading.
The title is the same as the issue (#85868) it corresponds to.
Got a look to other PR's and I thought that this is the usual naming convention.
Please refer to the commit and you will see all the modified files.

@joker-eph
Copy link
Collaborator

PR should be named according to what they are actually achieving, I'm not sure why the GitHub issue title is relevant?

@bulbazord
Copy link
Member

Please update the PR subject as its a lot more than just X86AsmParser.cpp

Hi @RKSimon

All the issues mentioned are fixed. The title of the PR is misleading. The title is the same as the issue (#85868) it corresponds to. Got a look to other PR's and I thought that this is the usual naming convention. Please refer to the commit and you will see all the modified files.

Commit titles should accurately reflect a change. Your change modifies more than just X86AsmParser.cpp, so reading the commit title alone might lead one to believe that's all that changed. But it's not, you changed many things across the code base (even if the changes are all of the same kind). A more accurate title might be something like: Add clarifying parenthesis around non-trivial conditions in ternary expressions.

Copy link
Member

@ldionne ldionne left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The libc++abi changes look fine to me.

@lntue
Copy link
Contributor

lntue commented Apr 29, 2024

The title is the same as the issue (#85868) it corresponds to.

You can update the title of the PR to reflect the changes, and add "Fixes #85868" to the PR description to relate it to the issue.

OTOH, the libc changes look good to me.

@luolent luolent changed the title llvm/lib/Target/X86/AsmParser/X86AsmParser.cpp:3804: lacking () for c… Add clarifying parenthesis around non-trivial conditions in ternary expressions. Apr 30, 2024
@luolent
Copy link
Contributor Author

luolent commented Apr 30, 2024

Hi @bulbazord , @lntue ,

thanks for your helpful comments, first time contributing here :)
I have updated the PR.

@jayfoad
Copy link
Contributor

jayfoad commented Apr 30, 2024

AMDGPU changes are fine.

@bulbazord
Copy link
Member

LLDB changes look fine.

Copy link
Collaborator

@RKSimon RKSimon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@RKSimon RKSimon merged commit a98a6e9 into llvm:main May 4, 2024
49 of 50 checks passed
Copy link

github-actions bot commented May 4, 2024

@luolent Congratulations on having your first Pull Request (PR) merged into the LLVM Project!

Your changes will be combined with recent changes from other authors, then tested
by our build bots. If there is a problem with a build, you may receive a report in an email or a comment on this PR.

Please check whether problems have been caused by your change specifically, as
the builds can include changes from many authors. It is not uncommon for your
change to be included in a build that fails due to someone else's changes, or
infrastructure issues.

How to do this, and the rest of the post-merge process, is covered in detail here.

If your change does cause a problem, it may be reverted, or you can revert it yourself.
This is a normal part of LLVM development. You can fix your changes and open a new PR to merge them again.

If you don't get any reports, no action is required from you. Your changes are working as expected, well done!

@@ -3802,7 +3802,7 @@ bool X86AsmParser::validateInstruction(MCInst &Inst, const OperandVector &Ops) {
// VFMULCPHZrr Dest, Src1, Src2
// VFMULCPHZrrk Dest, Dest, Mask, Src1, Src2
// VFMULCPHZrrkz Dest, Mask, Src1, Src2
for (unsigned i = TSFlags & X86II::EVEX_K ? 2 : 1;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know it's subjective, but the new code with two levels of parens decreases readability.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The second level seems like a bug, it's not consistent with how type identifier = expr ? expr : expr; is handled in a block.

@@ -801,7 +801,7 @@ static OutputDesc *addInputSec(StringMap<TinyPtrVector<OutputSection *>> &map,
auto *firstIsec = cast<InputSectionBase>(
cast<InputSectionDescription>(sec->commands[0])->sectionBases[0]);
OutputSection *firstIsecOut =
firstIsec->flags & SHF_LINK_ORDER
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is random change made to lld/ELF?

[-+*%] \w+ \?' is extensively used and I don't see we add )

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @MaskRay ,

the PR modifies all the files with non-trivial ternary operators given by Cppcheck (issue).
You can find the Cppcheck report inside the comments thread of the issue.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A CppCheck issue may or may not be useful for the project. Projects have different code styles. Applying a random static analyzer report may not be useful.

sookach pushed a commit to sookach/llvm-project that referenced this pull request May 4, 2024
…xpressions. (llvm#90391)

Fixes [llvm#85868](llvm#85868)

Parenthesis are added as requested on ternary operators with non trivial conditions.

I used this [precedence table](https://en.cppreference.com/w/cpp/language/operator_precedence) for reference, to make sure we get the expected behavior on each change.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backend:AMDGPU backend:SystemZ backend:X86 clang:frontend Language frontend issues, e.g. anything involving "Sema" clang Clang issues not falling into any other category compiler-rt libc++abi libc++abi C++ Runtime Library. Not libc++. libc lld:ELF lld lldb mlir:tosa mlir xray
Projects
None yet
Development

Successfully merging this pull request may close these issues.

llvm/lib/Target/X86/AsmParser/X86AsmParser.cpp:3804: lacking () for clarity
10 participants