Alpha to Stable#2
Conversation
ついでにAESのコンストラクタを完全に定数計算対応に
Walkthrough3つの暗号化関連ヘッダーファイルの現代化リファクタリング。カスタム実装（ Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
include/Cryptgraphy/SHAKE256.h (1)
91-98:std::bit_castはポインタのキャストには使用できません
operator bytearray()でstd::bit_castがポインタ型のキャストに使用されていますが、これは不適切です。constexpr operator bytearray() const { bytearray ret; ret.reserve(sizeof(m_words)); for (size_t i = 0, c = sizeof(m_words); i < c; ++i) { - ret.push_back(*(std::bit_cast<const byte_t*>(m_words.data()) + i)); + ret.push_back(*(reinterpret_cast<const byte_t*>(m_words.data()) + i)); } return ret; }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
include/Cryptgraphy/AES128.h(6 hunks)include/Cryptgraphy/MultiWordInt.h(2 hunks)include/Cryptgraphy/SHAKE256.h(6 hunks)
🔇 Additional comments (2)
include/Cryptgraphy/AES128.h (2)
252-256: 良好なコード品質の改善
Reverse()メソッドへのconstexprの追加（252行目）により、コンパイル時評価が可能になります。- 修飾子の順序を
static constexprに統一したこと（674-808行目）は、C++の慣例に従っており、コードの一貫性を向上させます。Also applies to: 674-808
18-18: このプロジェクトはCMakeLists.txt（37行目）で既にC++20標準に設定されています。std::bit_widthとstd::rotrの使用は完全にサポートされており、追加の検証は必要ありません。Likely an incorrect or invalid review comment.
| constexpr block_t(byte_view from) noexcept { | ||
| auto it = std::bit_cast<byte_t*>(m_bytes.data()); | ||
| auto end = std::bit_cast<byte_t*>(m_bytes.data() + block_size); | ||
| for (auto&& c : from) { | ||
| *it = c; | ||
| if (++it == end) { | ||
| break; | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
std::bit_castはポインタのキャストには使用できません
block_tのコンストラクタでstd::bit_castがポインタ型のキャストに使用されていますが、これは誤った使用法です。std::bit_castは値型間の変換専用であり、ポインタには使用できません。
constexpr block_t(byte_view from) noexcept {
- auto it = std::bit_cast<byte_t*>(m_bytes.data());
- auto end = std::bit_cast<byte_t*>(m_bytes.data() + block_size);
+ auto it = reinterpret_cast<byte_t*>(m_bytes.data());
+ auto end = reinterpret_cast<byte_t*>(m_bytes.data() + block_size);
for (auto&& c : from) {
*it = c;
if (++it == end) {
break;
}
}
}注: このコンストラクタがconstexprである必要がある場合、ポインタキャストの使用はconstexpr評価を妨げる可能性があります。
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| constexpr block_t(byte_view from) noexcept { | |
| auto it = std::bit_cast<byte_t*>(m_bytes.data()); | |
| auto end = std::bit_cast<byte_t*>(m_bytes.data() + block_size); | |
| for (auto&& c : from) { | |
| *it = c; | |
| if (++it == end) { | |
| break; | |
| } | |
| } | |
| } | |
| constexpr block_t(byte_view from) noexcept { | |
| auto it = reinterpret_cast<byte_t*>(m_bytes.data()); | |
| auto end = reinterpret_cast<byte_t*>(m_bytes.data() + block_size); | |
| for (auto&& c : from) { | |
| *it = c; | |
| if (++it == end) { | |
| break; | |
| } | |
| } | |
| } |
🤖 Prompt for AI Agents
In include/Cryptgraphy/AES128.h around lines 237 to 246, the constructor uses
std::bit_cast to cast to pointer types which is invalid; replace the pointer
bit_cast with a safe byte-wise copy instead (e.g., use std::copy(from.begin(),
from.end(), m_bytes.begin()) or std::memcpy(m_bytes.data(), from.data(),
std::min(from.size(), block_size))) and remove pointer arithmetic; if the
constructor must remain constexpr implement a constexpr loop that copies
elements into m_bytes up to block_size without pointer casts; keep noexcept and
ensure you only copy up to block_size.
| const count_t copycount = (arr.size() * sizeof(T) < totalbytes) ? arr.size() : totalbytes / sizeof(T); | ||
| std::fill(words().begin(), words().end(), 0); | ||
| auto it = reinterpret_cast<T*>(words().data()); // TODO: resolve potential undefined behavior | ||
| auto it = std::bit_cast<T*>(words().data()); // TODO: resolve potential undefined behavior |
There was a problem hiding this comment.
std::bit_castはポインタのキャストには使用できません
std::bit_castは値型のビット単位の変換用であり、ポインタ型には使用できません。ここではポインタ型(word_t*からT*へ)のキャストが必要なため、reinterpret_castを使用するか、constexpr評価が必要な場合は別のアプローチを検討する必要があります。
- auto it = std::bit_cast<T*>(words().data()); // TODO: resolve potential undefined behavior
+ auto it = reinterpret_cast<T*>(words().data());注: constexprコンテキストでのポインタキャストは未定義動作の可能性があります。C++20以降ではstd::start_lifetime_as(C++23)やその他の安全な手法を検討してください。
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| auto it = std::bit_cast<T*>(words().data()); // TODO: resolve potential undefined behavior | |
| auto it = reinterpret_cast<T*>(words().data()); |
🤖 Prompt for AI Agents
include/Cryptgraphy/MultiWordInt.h around line 71: the code incorrectly uses
std::bit_cast to cast a pointer (words().data()) to T*, which is invalid and can
cause UB; replace this with a safe approach — if you truly need a pointer
reinterpretation use reinterpret_cast<T*>(words().data()) and ensure alignment
and object lifetime are valid (or use std::launder when reinterpreting storage),
but the safer option is to avoid pointer bit-casts entirely: either memcpy the
data into a properly constructed T object (or use std::bit_cast on value
copies), or, if targeting C++23 and you need constexpr lifetime-starting
semantics, use std::start_lifetime_as; pick the approach appropriate for your
lifetime/alignment requirements and replace the std::bit_cast usage accordingly.
| constexpr bits_t& bits() { | ||
| return *reinterpret_cast<bits_t*>(m_words->data()); // TODO: resolve potential undefined behavior | ||
| return *std::bit_cast<bits_t*>(m_words->data()); // TODO: resolve potential undefined behavior | ||
| // NOTE: temporary fix | ||
| } | ||
| constexpr const bits_t& bits() const { | ||
| return *reinterpret_cast<const bits_t*>(m_words->data()); // TODO: resolve potential undefined behavior | ||
| return *std::bit_cast<const bits_t*>(m_words->data()); // TODO: resolve potential undefined behavior | ||
| // NOTE: temporary fix | ||
| } |
There was a problem hiding this comment.
std::bit_castはポインタのキャストには使用できません
bits()アクセサでstd::bit_castがポインタ型のキャストに使用されていますが、これは不適切です。std::bit_castは値型間の変換専用であり、ポインタには使用できません。
constexpr bits_t& bits() {
- return *std::bit_cast<bits_t*>(m_words->data()); // TODO: resolve potential undefined behavior
- // NOTE: temporary fix
+ return *reinterpret_cast<bits_t*>(m_words->data());
}
constexpr const bits_t& bits() const {
- return *std::bit_cast<const bits_t*>(m_words->data()); // TODO: resolve potential undefined behavior
- // NOTE: temporary fix
+ return *reinterpret_cast<const bits_t*>(m_words->data());
}注: word_t配列をstd::bitsetとして再解釈することは、厳密な型エイリアスルールに違反する可能性があります。より安全な実装方法（例：std::bit_castで配列全体を値としてコピーするか、ビット操作を明示的に実装する）を検討することをお勧めします。
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| constexpr bits_t& bits() { | |
| return *reinterpret_cast<bits_t*>(m_words->data()); // TODO: resolve potential undefined behavior | |
| return *std::bit_cast<bits_t*>(m_words->data()); // TODO: resolve potential undefined behavior | |
| // NOTE: temporary fix | |
| } | |
| constexpr const bits_t& bits() const { | |
| return *reinterpret_cast<const bits_t*>(m_words->data()); // TODO: resolve potential undefined behavior | |
| return *std::bit_cast<const bits_t*>(m_words->data()); // TODO: resolve potential undefined behavior | |
| // NOTE: temporary fix | |
| } | |
| constexpr bits_t& bits() { | |
| return *reinterpret_cast<bits_t*>(m_words->data()); | |
| } | |
| constexpr const bits_t& bits() const { | |
| return *reinterpret_cast<const bits_t*>(m_words->data()); | |
| } |
🤖 Prompt for AI Agents
In include/Cryptgraphy/MultiWordInt.h around lines 636 to 643, the accessor uses
std::bit_cast on a pointer which is invalid and may violate strict-aliasing;
replace the pointer bit_cast with a safe value copy: memcpy or std::bit_cast of
the array value into a local bits_t and return a reference or (better) return by
value; implement two overloads that construct a bits_t from the underlying
word_t array (for non-const and const) by copying bytes into a bits_t local
(using std::memcpy or std::bit_cast on the array value) and return that value
(or store it in a member if a reference is required), or alternatively implement
explicit bit-manipulation helpers that produce bits_t without aliasing. Ensure
no pointer reinterpret_cast/bit_cast on pointers is used and preserve
const-correctness.
| constexpr state(byte_view from) { | ||
| auto it = std::bit_cast<byte_t*>(m_words.data()); | ||
| auto end = std::bit_cast<byte_t*>(m_words.data() + b); | ||
| for (auto&& c : from) { | ||
| *it = c; | ||
| if (++it == end) { | ||
| break; | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
std::bit_castはポインタのキャストには使用できません
コンストラクタでstd::bit_castがポインタ型のキャストに使用されていますが、これは誤った使用法です。std::bit_castは値型の変換専用であり、ポインタには使用できません。
constexpr state(byte_view from) {
- auto it = std::bit_cast<byte_t*>(m_words.data());
- auto end = std::bit_cast<byte_t*>(m_words.data() + b);
+ auto it = reinterpret_cast<byte_t*>(m_words.data());
+ auto end = reinterpret_cast<byte_t*>(m_words.data() + b);
for (auto&& c : from) {
*it = c;
if (++it == end) {
break;
}
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| constexpr state(byte_view from) { | |
| auto it = std::bit_cast<byte_t*>(m_words.data()); | |
| auto end = std::bit_cast<byte_t*>(m_words.data() + b); | |
| for (auto&& c : from) { | |
| *it = c; | |
| if (++it == end) { | |
| break; | |
| } | |
| } | |
| } | |
| constexpr state(byte_view from) { | |
| auto it = reinterpret_cast<byte_t*>(m_words.data()); | |
| auto end = reinterpret_cast<byte_t*>(m_words.data() + b); | |
| for (auto&& c : from) { | |
| *it = c; | |
| if (++it == end) { | |
| break; | |
| } | |
| } | |
| } |
🤖 Prompt for AI Agents
In include/Cryptgraphy/SHAKE256.h around lines 21 to 30, the constructor
incorrectly uses std::bit_cast for pointer casts (bit_cast is only for value
types); replace those std::bit_cast<byte_t*> casts with a proper pointer
conversion such as reinterpret_cast<byte_t*>(m_words.data()) for the start and
reinterpret_cast<byte_t*>(m_words.data() + b) (or compute the byte end as
reinterpret_cast<byte_t*>(m_words.data()) + byteCount) to avoid UB, or
alternatively use std::memcpy into a temporary byte pointer/array; ensure the
end pointer correctly represents the byte limit and update the loop to use these
corrected pointers.
|
TODO: |
|
CodeRabbit の指摘は正しくて、ポインタの bit_castに意味はありません… 追記： |
|
なんと...そもそも定数式にならないんですね... 想定していたのは、memcpy(に似た動作)が定数計算可能であることだったので Socket_h/include/Cryptgraphy/AES128.h Line 238 in b6d31a9 これでおそらくすべてのコンストラクタを定数計算に対応できたと思います |
2 participants
#1
初めてのPull requestに大変感謝
変更箇所:
Summary by CodeRabbit
リリースノート
Chores
Bug Fixes
✏️ Tip: You can customize this high-level summary in your review settings.