[AST] Give CharUnits::operator% a consistent type. NFC
#160781
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Update the `operator%` overload that accepts `CharUnits` to return `CharUnits` to match the other `operator%`. This is more logical than returning an `int64` and cleans up users that want to continue to do math with the result. Many users of this were explicitly comparing against 0. I considered updating these to compare against `CharUnits::Zero` or even introducing an `explicit operator bool()`, but they all feel clearer if we update them to use the existing `isMultipleOf()` function instead.
llvmbot
added
clang
|
@llvm/pr-subscribers-clang @llvm/pr-subscribers-clang-codegen Author: Justin Bogner (bogner) ChangesUpdate the Many users of this were explicitly comparing against 0. I considered updating these to compare against Full diff: https://github.com/llvm/llvm-project/pull/160781.diff 9 Files Affected:
diff --git a/clang/include/clang/AST/CharUnits.h b/clang/include/clang/AST/CharUnits.h
index c06354451dfbe..bd7457f1caeaa 100644
--- a/clang/include/clang/AST/CharUnits.h
+++ b/clang/include/clang/AST/CharUnits.h
@@ -141,7 +141,7 @@ namespace clang {
/// Among other things, this promises that
/// self.alignTo(N) will just return self.
bool isMultipleOf(CharUnits N) const {
- return (*this % N) == 0;
+ return (*this % N) == CharUnits::Zero();
}
// Arithmetic operators.
@@ -165,8 +165,8 @@ namespace clang {
CharUnits operator% (QuantityType N) const {
return CharUnits(Quantity % N);
}
- QuantityType operator% (const CharUnits &Other) const {
- return Quantity % Other.Quantity;
+ CharUnits operator% (const CharUnits &Other) const {
+ return CharUnits(Quantity % Other.Quantity);
}
CharUnits operator+ (const CharUnits &Other) const {
return CharUnits(Quantity + Other.Quantity);
diff --git a/clang/lib/AST/APValue.cpp b/clang/lib/AST/APValue.cpp
index 7173c2a0e1a2a..2e1c8eb3726cf 100644
--- a/clang/lib/AST/APValue.cpp
+++ b/clang/lib/AST/APValue.cpp
@@ -784,7 +784,7 @@ void APValue::printPretty(raw_ostream &Out, const PrintingPolicy &Policy,
if (!O.isZero()) {
if (IsReference)
Out << "*(";
- if (S.isZero() || O % S) {
+ if (S.isZero() || !O.isMultipleOf(S)) {
Out << "(char*)";
S = CharUnits::One();
}
diff --git a/clang/lib/AST/RecordLayoutBuilder.cpp b/clang/lib/AST/RecordLayoutBuilder.cpp
index 43f4e070748bb..00b938bdf308d 100644
--- a/clang/lib/AST/RecordLayoutBuilder.cpp
+++ b/clang/lib/AST/RecordLayoutBuilder.cpp
@@ -2087,9 +2087,8 @@ void ItaniumRecordLayoutBuilder::LayoutField(const FieldDecl *D,
if (InsertExtraPadding) {
CharUnits ASanAlignment = CharUnits::fromQuantity(8);
CharUnits ExtraSizeForAsan = ASanAlignment;
- if (FieldSize % ASanAlignment)
- ExtraSizeForAsan +=
- ASanAlignment - CharUnits::fromQuantity(FieldSize % ASanAlignment);
+ if (!FieldSize.isMultipleOf(ASanAlignment))
+ ExtraSizeForAsan += ASanAlignment - (FieldSize % ASanAlignment);
EffectiveFieldSize = FieldSize = FieldSize + ExtraSizeForAsan;
}
@@ -2119,10 +2118,10 @@ void ItaniumRecordLayoutBuilder::LayoutField(const FieldDecl *D,
if (RD->hasAttr<PackedAttr>() || !MaxFieldAlignment.isZero())
if (FieldAlign < OriginalFieldAlign)
if (D->getType()->isRecordType()) {
- // If the offset is a multiple of the alignment of
+ // If the offset is not a multiple of the alignment of
// the type, raise the warning.
// TODO: Takes no account the alignment of the outer struct
- if (FieldOffset % OriginalFieldAlign != 0)
+ if (!FieldOffset.isMultipleOf(OriginalFieldAlign))
Diag(D->getLocation(), diag::warn_unaligned_access)
<< Context.getCanonicalTagType(RD) << D->getName()
<< D->getType();
diff --git a/clang/lib/CodeGen/CGAtomic.cpp b/clang/lib/CodeGen/CGAtomic.cpp
index 9106c4cd8e139..64e15c5b86656 100644
--- a/clang/lib/CodeGen/CGAtomic.cpp
+++ b/clang/lib/CodeGen/CGAtomic.cpp
@@ -879,7 +879,7 @@ RValue CodeGenFunction::EmitAtomicExpr(AtomicExpr *E) {
CharUnits MaxInlineWidth =
getContext().toCharUnitsFromBits(MaxInlineWidthInBits);
DiagnosticsEngine &Diags = CGM.getDiags();
- bool Misaligned = (Ptr.getAlignment() % TInfo.Width) != 0;
+ bool Misaligned = !Ptr.getAlignment().isMultipleOf(TInfo.Width);
bool Oversized = getContext().toBits(TInfo.Width) > MaxInlineWidthInBits;
if (Misaligned) {
Diags.Report(E->getBeginLoc(), diag::warn_atomic_op_misaligned)
diff --git a/clang/lib/CodeGen/CGExprConstant.cpp b/clang/lib/CodeGen/CGExprConstant.cpp
index b44dd9ecc717e..6407afc3d9447 100644
--- a/clang/lib/CodeGen/CGExprConstant.cpp
+++ b/clang/lib/CodeGen/CGExprConstant.cpp
@@ -433,7 +433,7 @@ llvm::Constant *ConstantAggregateBuilder::buildFrom(
// All remaining elements must be the same type.
if (Elems[I]->getType() != CommonType ||
- Offset(I) % ElemSize != 0) {
+ !Offset(I).isMultipleOf(ElemSize)) {
CanEmitArray = false;
break;
}
diff --git a/clang/lib/CodeGen/CGObjCMac.cpp b/clang/lib/CodeGen/CGObjCMac.cpp
index 60f30a1334d6d..dbcce9b86ad52 100644
--- a/clang/lib/CodeGen/CGObjCMac.cpp
+++ b/clang/lib/CodeGen/CGObjCMac.cpp
@@ -5367,7 +5367,7 @@ IvarLayoutBuilder::buildBitmap(CGObjCCommonMac &CGObjC,
// Ignore scan requests that don't start at an even multiple of the
// word size. We can't encode them.
- if ((beginOfScan % WordSize) != 0)
+ if (!beginOfScan.isMultipleOf(WordSize))
continue;
// Ignore scan requests that start before the instance start.
diff --git a/clang/lib/CodeGen/CGRecordLayoutBuilder.cpp b/clang/lib/CodeGen/CGRecordLayoutBuilder.cpp
index 5f6136c917ac2..e9205c68c2812 100644
--- a/clang/lib/CodeGen/CGRecordLayoutBuilder.cpp
+++ b/clang/lib/CodeGen/CGRecordLayoutBuilder.cpp
@@ -369,11 +369,11 @@ void CGRecordLowering::lowerUnion(bool isNonVirtualBaseType) {
appendPaddingBytes(LayoutSize - getSize(StorageType));
// Set packed if we need it.
const auto StorageAlignment = getAlignment(StorageType);
- assert((Layout.getSize() % StorageAlignment == 0 ||
- Layout.getDataSize() % StorageAlignment) &&
+ assert((Layout.getSize().isMultipleOf(StorageAlignment) ||
+ !Layout.getDataSize().isMultipleOf(StorageAlignment)) &&
"Union's standard layout and no_unique_address layout must agree on "
"packedness");
- if (Layout.getDataSize() % StorageAlignment)
+ if (!Layout.getDataSize().isMultipleOf(StorageAlignment))
Packed = true;
}
@@ -977,7 +977,7 @@ void CGRecordLowering::determinePacked(bool NVBaseType) {
continue;
// If any member falls at an offset that it not a multiple of its alignment,
// then the entire record must be packed.
- if (Member.Offset % getAlignment(Member.Data))
+ if (!Member.Offset.isMultipleOf(getAlignment(Member.Data)))
Packed = true;
if (Member.Offset < NVSize)
NVAlignment = std::max(NVAlignment, getAlignment(Member.Data));
@@ -985,12 +985,12 @@ void CGRecordLowering::determinePacked(bool NVBaseType) {
}
// If the size of the record (the capstone's offset) is not a multiple of the
// record's alignment, it must be packed.
- if (Members.back().Offset % Alignment)
+ if (!Members.back().Offset.isMultipleOf(Alignment))
Packed = true;
// If the non-virtual sub-object is not a multiple of the non-virtual
// sub-object's alignment, it must be packed. We cannot have a packed
// non-virtual sub-object and an unpacked complete object or vise versa.
- if (NVSize % NVAlignment)
+ if (!NVSize.isMultipleOf(NVAlignment))
Packed = true;
// Update the alignment of the sentinel.
if (!Packed)
diff --git a/clang/lib/Sema/SemaChecking.cpp b/clang/lib/Sema/SemaChecking.cpp
index 27e3456bc6681..c5fdc6f9a5c81 100644
--- a/clang/lib/Sema/SemaChecking.cpp
+++ b/clang/lib/Sema/SemaChecking.cpp
@@ -15888,7 +15888,7 @@ void Sema::RefersToMemberWithReducedAlignment(
}
// Check if the synthesized offset fulfills the alignment.
- if (Offset % ExpectedAlignment != 0 ||
+ if (!Offset.isMultipleOf(ExpectedAlignment) ||
// It may fulfill the offset it but the effective alignment may still be
// lower than the expected expression alignment.
CompleteObjectAlignment < ExpectedAlignment) {
diff --git a/clang/lib/StaticAnalyzer/Core/Store.cpp b/clang/lib/StaticAnalyzer/Core/Store.cpp
index 971e6bc798837..b609f36ae7e89 100644
--- a/clang/lib/StaticAnalyzer/Core/Store.cpp
+++ b/clang/lib/StaticAnalyzer/Core/Store.cpp
@@ -210,7 +210,7 @@ std::optional<const MemRegion *> StoreManager::castRegion(const MemRegion *R,
// Is the offset a multiple of the size? If so, we can layer the
// ElementRegion (with elementType == PointeeTy) directly on top of
// the base region.
- if (off % pointeeTySize == 0) {
+ if (off.isMultipleOf(pointeeTySize)) {
newIndex = off / pointeeTySize;
newSuperR = baseR;
}
|
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
cor3ntin
approved these changes
Sep 28, 2025
mahesh-attarde
pushed a commit
to mahesh-attarde/llvm-project
that referenced
this pull request
Oct 3, 2025
Update the `operator%` overload that accepts `CharUnits` to return `CharUnits` to match the other `operator%`. This is more logical than returning an `int64` and cleans up users that want to continue to do math with the result. Many users of this were explicitly comparing against 0. I considered updating these to compare against `CharUnits::Zero` or even introducing an `explicit operator bool()`, but they all feel clearer if we update them to use the existing `isMultipleOf()` function instead.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Update the
operator%overload that acceptsCharUnitsto returnCharUnitsto match the otheroperator%. This is more logical than returning anint64and cleans up users that want to continue to do math with the result.Many users of this were explicitly comparing against 0. I considered updating these to compare against
CharUnits::Zeroor even introducing anexplicit operator bool(), but they all feel clearer if we update them to use the existingisMultipleOf()function instead.