From ab8c25123963998833635a4b6c4f072009473fda Mon Sep 17 00:00:00 2001 From: Patil Date: Mon, 23 Mar 2026 21:10:24 +0530 Subject: [PATCH 1/2] Add propelcode test conditions in score/evaluation --- score/evaluation/BUILD | 119 ++++++++ score/evaluation/EVALUATION_GUIDE.md | 29 ++ score/evaluation/concurrency/BUILD | 13 + .../concurrency/EVALUATION_GUIDE.md | 17 ++ .../concurrency/concurrency_issues.h | 208 ++++++++++++++ score/evaluation/design/BUILD | 13 + score/evaluation/design/EVALUATION_GUIDE.md | 19 ++ score/evaluation/design/design_faults.h | 240 ++++++++++++++++ score/evaluation/evaluation_main.cpp | 122 ++++++++ score/evaluation/exception_handling/BUILD | 13 + .../exception_handling/EVALUATION_GUIDE.md | 17 ++ .../exception_error_handling.h | 177 ++++++++++++ score/evaluation/idiomatic/BUILD | 13 + .../evaluation/idiomatic/EVALUATION_GUIDE.md | 20 ++ .../evaluation/idiomatic/non_idiomatic_cpp.h | 155 +++++++++++ score/evaluation/memory/BUILD | 13 + score/evaluation/memory/EVALUATION_GUIDE.md | 16 ++ score/evaluation/memory/memory_issues.h | 218 +++++++++++++++ score/evaluation/modern_cpp/BUILD | 13 + .../evaluation/modern_cpp/EVALUATION_GUIDE.md | 20 ++ .../evaluation/modern_cpp/modern_cpp_syntax.h | 179 ++++++++++++ score/evaluation/optimization/BUILD | 13 + .../optimization/EVALUATION_GUIDE.md | 20 ++ .../optimization/code_optimization.h | 167 +++++++++++ score/evaluation/patterns/BUILD | 13 + score/evaluation/patterns/EVALUATION_GUIDE.md | 16 ++ score/evaluation/patterns/design_patterns.h | 228 +++++++++++++++ score/evaluation/safety/BUILD | 13 + score/evaluation/safety/EVALUATION_GUIDE.md | 18 ++ score/evaluation/safety/safe_cpp.h | 158 +++++++++++ score/evaluation/security/BUILD | 13 + score/evaluation/security/EVALUATION_GUIDE.md | 18 ++ score/evaluation/security/security_issues.h | 131 +++++++++ score/evaluation/solid/BUILD | 13 + score/evaluation/solid/EVALUATION_GUIDE.md | 15 + score/evaluation/solid/solid_violations.h | 218 +++++++++++++++ .../evaluation/template_metaprogramming/BUILD | 13 + .../EVALUATION_GUIDE.md | 21 ++ .../template_metaprogramming_issues.h | 225 +++++++++++++++ score/evaluation/unit_test_quality/BUILD | 23 ++ .../unit_test_quality/EVALUATION_GUIDE.md | 24 ++ .../unit_test_quality/unit_test_quality.h | 233 ++++++++++++++++ .../unit_test_quality_test.cpp | 263 ++++++++++++++++++ 43 files changed, 3490 insertions(+) create mode 100644 score/evaluation/BUILD create mode 100644 score/evaluation/EVALUATION_GUIDE.md create mode 100644 score/evaluation/concurrency/BUILD create mode 100644 score/evaluation/concurrency/EVALUATION_GUIDE.md create mode 100644 score/evaluation/concurrency/concurrency_issues.h create mode 100644 score/evaluation/design/BUILD create mode 100644 score/evaluation/design/EVALUATION_GUIDE.md create mode 100644 score/evaluation/design/design_faults.h create mode 100644 score/evaluation/evaluation_main.cpp create mode 100644 score/evaluation/exception_handling/BUILD create mode 100644 score/evaluation/exception_handling/EVALUATION_GUIDE.md create mode 100644 score/evaluation/exception_handling/exception_error_handling.h create mode 100644 score/evaluation/idiomatic/BUILD create mode 100644 score/evaluation/idiomatic/EVALUATION_GUIDE.md create mode 100644 score/evaluation/idiomatic/non_idiomatic_cpp.h create mode 100644 score/evaluation/memory/BUILD create mode 100644 score/evaluation/memory/EVALUATION_GUIDE.md create mode 100644 score/evaluation/memory/memory_issues.h create mode 100644 score/evaluation/modern_cpp/BUILD create mode 100644 score/evaluation/modern_cpp/EVALUATION_GUIDE.md create mode 100644 score/evaluation/modern_cpp/modern_cpp_syntax.h create mode 100644 score/evaluation/optimization/BUILD create mode 100644 score/evaluation/optimization/EVALUATION_GUIDE.md create mode 100644 score/evaluation/optimization/code_optimization.h create mode 100644 score/evaluation/patterns/BUILD create mode 100644 score/evaluation/patterns/EVALUATION_GUIDE.md create mode 100644 score/evaluation/patterns/design_patterns.h create mode 100644 score/evaluation/safety/BUILD create mode 100644 score/evaluation/safety/EVALUATION_GUIDE.md create mode 100644 score/evaluation/safety/safe_cpp.h create mode 100644 score/evaluation/security/BUILD create mode 100644 score/evaluation/security/EVALUATION_GUIDE.md create mode 100644 score/evaluation/security/security_issues.h create mode 100644 score/evaluation/solid/BUILD create mode 100644 score/evaluation/solid/EVALUATION_GUIDE.md create mode 100644 score/evaluation/solid/solid_violations.h create mode 100644 score/evaluation/template_metaprogramming/BUILD create mode 100644 score/evaluation/template_metaprogramming/EVALUATION_GUIDE.md create mode 100644 score/evaluation/template_metaprogramming/template_metaprogramming_issues.h create mode 100644 score/evaluation/unit_test_quality/BUILD create mode 100644 score/evaluation/unit_test_quality/EVALUATION_GUIDE.md create mode 100644 score/evaluation/unit_test_quality/unit_test_quality.h create mode 100644 score/evaluation/unit_test_quality/unit_test_quality_test.cpp diff --git a/score/evaluation/BUILD b/score/evaluation/BUILD new file mode 100644 index 0000000..8bc02e7 --- /dev/null +++ b/score/evaluation/BUILD @@ -0,0 +1,119 @@ +# ******************************************************************************* +# Copyright (c) 2025 Contributors to the Eclipse Foundation +# +# See the NOTICE file(s) distributed with this work for additional +# information regarding copyright ownership. +# +# This program and the accompanying materials are made available under the +# terms of the Apache License Version 2.0 which is available at +# https://www.apache.org/licenses/LICENSE-2.0 +# +# SPDX-License-Identifier: Apache-2.0 +# ******************************************************************************* +# +# BUILD file for score/evaluation +# +# Each cc_library target corresponds to one evaluation scenario file. +# The cc_binary target "evaluation_all" links every library and provides +# a single compilable artefact that the code-review tool can analyse. +# +# NOTE: These targets intentionally contain anti-patterns and bad practices. +# They are NOT suitable for production use and exist solely for +# evaluating the capability of automated code-review tools. +# ******************************************************************************* + +load("@rules_cc//cc:defs.bzl", "cc_binary") + +alias( + name = "solid_violations", + actual = "//score/evaluation/solid:solid_violations", +) + +alias( + name = "memory_issues", + actual = "//score/evaluation/memory:memory_issues", +) + +alias( + name = "non_idiomatic_cpp", + actual = "//score/evaluation/idiomatic:non_idiomatic_cpp", +) + +alias( + name = "modern_cpp_syntax", + actual = "//score/evaluation/modern_cpp:modern_cpp_syntax", +) + +alias( + name = "design_patterns", + actual = "//score/evaluation/patterns:design_patterns", +) + +alias( + name = "code_optimization", + actual = "//score/evaluation/optimization:code_optimization", +) + +alias( + name = "safe_cpp", + actual = "//score/evaluation/safety:safe_cpp", +) + +alias( + name = "security_issues", + actual = "//score/evaluation/security:security_issues", +) + +alias( + name = "design_faults", + actual = "//score/evaluation/design:design_faults", +) + +alias( + name = "concurrency_issues", + actual = "//score/evaluation/concurrency:concurrency_issues", +) + +alias( + name = "exception_error_handling", + actual = "//score/evaluation/exception_handling:exception_error_handling", +) + +alias( + name = "unit_test_quality", + actual = "//score/evaluation/unit_test_quality:unit_test_quality", +) + +alias( + name = "template_metaprogramming_issues", + actual = "//score/evaluation/template_metaprogramming:template_metaprogramming_issues", +) + +alias( + name = "unit_test_quality_test", + actual = "//score/evaluation/unit_test_quality:unit_test_quality_test", +) + +# --------------------------------------------------------------------------- +# Aggregate binary – compiles all evaluation scenarios together. +# --------------------------------------------------------------------------- +cc_binary( + name = "evaluation_all", + srcs = ["evaluation_main.cpp"], + visibility = ["//visibility:public"], + deps = [ + "//score/evaluation/concurrency:concurrency_issues", + "//score/evaluation/design:design_faults", + "//score/evaluation/exception_handling:exception_error_handling", + "//score/evaluation/idiomatic:non_idiomatic_cpp", + "//score/evaluation/memory:memory_issues", + "//score/evaluation/modern_cpp:modern_cpp_syntax", + "//score/evaluation/optimization:code_optimization", + "//score/evaluation/patterns:design_patterns", + "//score/evaluation/security:security_issues", + "//score/evaluation/safety:safe_cpp", + "//score/evaluation/solid:solid_violations", + "//score/evaluation/template_metaprogramming:template_metaprogramming_issues", + "//score/evaluation/unit_test_quality:unit_test_quality", + ], +) diff --git a/score/evaluation/EVALUATION_GUIDE.md b/score/evaluation/EVALUATION_GUIDE.md new file mode 100644 index 0000000..99b5f71 --- /dev/null +++ b/score/evaluation/EVALUATION_GUIDE.md @@ -0,0 +1,29 @@ +# Evaluation Guide (Root Index) + +This root document is intentionally trimmed. +Use the per-directory guides for detailed test conditions and expected review comments. + +## Per-Directory Guides + +| Directory | Guide | +|---|---| +| `solid/` | [`solid/EVALUATION_GUIDE.md`](solid/EVALUATION_GUIDE.md) | +| `memory/` | [`memory/EVALUATION_GUIDE.md`](memory/EVALUATION_GUIDE.md) | +| `idiomatic/` | [`idiomatic/EVALUATION_GUIDE.md`](idiomatic/EVALUATION_GUIDE.md) | +| `modern_cpp/` | [`modern_cpp/EVALUATION_GUIDE.md`](modern_cpp/EVALUATION_GUIDE.md) | +| `patterns/` | [`patterns/EVALUATION_GUIDE.md`](patterns/EVALUATION_GUIDE.md) | +| `optimization/` | [`optimization/EVALUATION_GUIDE.md`](optimization/EVALUATION_GUIDE.md) | +| `security/` | [`security/EVALUATION_GUIDE.md`](security/EVALUATION_GUIDE.md) | +| `safety/` | [`safety/EVALUATION_GUIDE.md`](safety/EVALUATION_GUIDE.md) | +| `design/` | [`design/EVALUATION_GUIDE.md`](design/EVALUATION_GUIDE.md) | +| `concurrency/` | [`concurrency/EVALUATION_GUIDE.md`](concurrency/EVALUATION_GUIDE.md) | +| `exception_handling/` | [`exception_handling/EVALUATION_GUIDE.md`](exception_handling/EVALUATION_GUIDE.md) | +| `template_metaprogramming/` | [`template_metaprogramming/EVALUATION_GUIDE.md`](template_metaprogramming/EVALUATION_GUIDE.md) | +| `unit_test_quality/` | [`unit_test_quality/EVALUATION_GUIDE.md`](unit_test_quality/EVALUATION_GUIDE.md) | + +## How to Use + +1. Review code changes folder-by-folder. +2. Open the corresponding local `EVALUATION_GUIDE.md`. +3. Compare tool comments against expected findings in that local guide. +4. Record coverage per tag (`SV-*`, `MI-*`, `UT-*`, etc.) in your review notes. diff --git a/score/evaluation/concurrency/BUILD b/score/evaluation/concurrency/BUILD new file mode 100644 index 0000000..7b6cfd0 --- /dev/null +++ b/score/evaluation/concurrency/BUILD @@ -0,0 +1,13 @@ +# ******************************************************************************* +# Copyright (c) 2025 Contributors to the Eclipse Foundation +# +# SPDX-License-Identifier: Apache-2.0 +# ******************************************************************************* + +load("@rules_cc//cc:defs.bzl", "cc_library") + +cc_library( + name = "concurrency_issues", + hdrs = ["concurrency_issues.h"], + visibility = ["//visibility:public"], +) diff --git a/score/evaluation/concurrency/EVALUATION_GUIDE.md b/score/evaluation/concurrency/EVALUATION_GUIDE.md new file mode 100644 index 0000000..0bec7d0 --- /dev/null +++ b/score/evaluation/concurrency/EVALUATION_GUIDE.md @@ -0,0 +1,17 @@ +# Concurrency Evaluation Guide + +## Scope +- **Source file:** `concurrency_issues.h` +- **Goal:** Verify tool catches race conditions, synchronization mistakes, and thread-lifetime bugs. + +## Test Conditions and Expected Review Comments +- **CI-01:** Data race on plain shared counter. +- **CI-02:** Manual lock/unlock without RAII (exception safety deadlock risk). +- **CI-03:** Broken double-checked init pattern / memory-ordering concerns. +- **CI-04:** Deadlock from inconsistent lock ordering. +- **CI-05:** Condition-variable wait not predicate-safe for spurious wakeups. +- **CI-06:** Detached thread captures stack reference (dangling reference risk). +- **CI-07:** Check-then-act race despite atomic variable. + +## Pass Criteria +- Tool should identify concrete interleavings/failure modes and propose robust synchronization patterns (`lock_guard`, `scoped_lock`, predicate waits, CAS loops). diff --git a/score/evaluation/concurrency/concurrency_issues.h b/score/evaluation/concurrency/concurrency_issues.h new file mode 100644 index 0000000..1a6d87b --- /dev/null +++ b/score/evaluation/concurrency/concurrency_issues.h @@ -0,0 +1,208 @@ +// ******************************************************************************* +// Copyright (c) 2025 Contributors to the Eclipse Foundation +// +// See the NOTICE file(s) distributed with this work for additional +// information regarding copyright ownership. +// +// This program and the accompanying materials are made available under the +// terms of the Apache License Version 2.0 which is available at +// https://www.apache.org/parameters/LICENSE-2.0 +// +// SPDX-License-Identifier: Apache-2.0 +// ******************************************************************************* + +/// @file concurrency_issues.h +/// @brief Concurrency and thread-safety issues for code-review tool evaluation. +/// +/// Issues planted (see EVALUATION_GUIDE.md §9 for expected review comments): +/// [CI-01] Data race: shared counter incremented without synchronisation. +/// [CI-02] Mutex locked but not protected by RAII lock guard – leak on exception. +/// [CI-03] Double-checked locking without std::atomic – broken on modern hardware. +/// [CI-04] Deadlock: two mutexes acquired in inconsistent order across two functions. +/// [CI-05] Condition variable spurious wakeup not handled (while replaced by if). +/// [CI-06] Detached thread writing to stack variable of the spawning scope. +/// [CI-07] std::atomic used for a compound check-then-act – still a TOCTOU race. + +#ifndef SCORE_EVALUATION_CONCURRENCY_ISSUES_H +#define SCORE_EVALUATION_CONCURRENCY_ISSUES_H + +#include +#include +#include +#include +#include + +namespace score +{ +namespace evaluation +{ + +// --------------------------------------------------------------------------- +// [CI-01] Data race on shared counter – no synchronisation. +// --------------------------------------------------------------------------- +class UnsafeCounter +{ +public: + void Increment() { ++count_; } // [CI-01] non-atomic, non-locked; data race + int Get() const { return count_; } + +private: + int count_{0}; // should be std::atomic or guarded by a mutex +}; + +// --------------------------------------------------------------------------- +// [CI-02] Manual mutex lock/unlock without RAII – leak on exception. +// --------------------------------------------------------------------------- +class ManualLockUser +{ +public: + void Write(const std::string& data) + { + mtx_.lock(); // [CI-02] if anything between lock and unlock throws, + ProcessData(data); // the mutex is never unlocked → deadlock. + mtx_.unlock(); // Use std::lock_guard lk(mtx_); + } + +private: + static void ProcessData(const std::string& /*data*/) + { + // may throw + } + + std::mutex mtx_; +}; + +// --------------------------------------------------------------------------- +// [CI-03] Double-checked locking without proper atomics / memory fences. +// --------------------------------------------------------------------------- +class BrokenLazyInit +{ +public: + // [CI-03] initialised_ is a plain bool – reads/writes are not atomic. + // A second thread may observe initialised_ == true before the + // constructor of resource_ has completed (memory reorder). + // Use std::atomic with acquire/release semantics or call_once. + void EnsureInitialised() + { + if (!initialised_) // first check, no lock + { + std::lock_guard lk(mtx_); + if (!initialised_) // second check, under lock + { + resource_ = 42; + initialised_ = true; // reorder: may be visible before resource_ = 42 + } + } + } + + int GetResource() const { return resource_; } + +private: + bool initialised_{false}; // [CI-03] should be std::atomic + int resource_{0}; + std::mutex mtx_; +}; + +// --------------------------------------------------------------------------- +// [CI-04] Deadlock: mutexes acquired in opposite order in two different paths. +// --------------------------------------------------------------------------- +class DeadlockProne +{ +public: + // Thread A calls TransferAB, Thread B calls TransferBA simultaneously → deadlock. + void TransferAB(int amount) + { + std::lock_guard la(mtx_a_); // locks A first + std::lock_guard lb(mtx_b_); // then B + balance_a_ -= amount; + balance_b_ += amount; + } + + void TransferBA(int amount) + { + std::lock_guard lb(mtx_b_); // locks B first [CI-04] + std::lock_guard la(mtx_a_); // then A – opposite order + balance_b_ -= amount; + balance_a_ += amount; + } + +private: + std::mutex mtx_a_; + std::mutex mtx_b_; + int balance_a_{1000}; + int balance_b_{1000}; +}; + +// --------------------------------------------------------------------------- +// [CI-05] Condition variable spurious wakeup handled with 'if' instead of 'while'. +// --------------------------------------------------------------------------- +class SpuriousWakeupBug +{ +public: + void Wait() + { + std::unique_lock lk(mtx_); + // [CI-05] Using if: spurious wakeup proceeds even when ready_ is false. + // Must use while (!ready_) or the predicate overload of wait(). + if (!ready_) cv_.wait(lk); + // Proceed – but ready_ might still be false after a spurious wakeup. + } + + void Signal() + { + std::lock_guard lk(mtx_); + ready_ = true; + cv_.notify_one(); + } + +private: + std::mutex mtx_; + std::condition_variable cv_; + bool ready_{false}; +}; + +// --------------------------------------------------------------------------- +// [CI-06] Detached thread capturing reference to local stack variable. +// --------------------------------------------------------------------------- +inline void StartDetachedThread() +{ + std::string local_config = "config_data"; + + // [CI-06] local_config is on the stack of StartDetachedThread(). + // After the function returns, the thread holds a dangling reference. + std::thread t([&local_config]() + { + // Accessing local_config after StartDetachedThread() returns → UB. + (void)local_config; + }); + t.detach(); + // Function returns here; local_config destroyed; thread still running. +} + +// --------------------------------------------------------------------------- +// [CI-07] Compound check-then-act on std::atomic is still a TOCTOU race. +// --------------------------------------------------------------------------- +class TocTouAtomic +{ +public: + // [CI-07] Even though count_ is atomic, the if-check and decrement are + // two separate operations. Another thread can decrement between the + // check and the decrement, driving count_ below zero. + void ConsumeIfAvailable() + { + if (count_.load() > 0) // check + { + --count_; // act – not atomic with the check + } + } + + void Produce() { ++count_; } + +private: + std::atomic count_{0}; +}; + +} // namespace evaluation +} // namespace score + +#endif // SCORE_EVALUATION_CONCURRENCY_ISSUES_H diff --git a/score/evaluation/design/BUILD b/score/evaluation/design/BUILD new file mode 100644 index 0000000..f06fa74 --- /dev/null +++ b/score/evaluation/design/BUILD @@ -0,0 +1,13 @@ +# ******************************************************************************* +# Copyright (c) 2025 Contributors to the Eclipse Foundation +# +# SPDX-License-Identifier: Apache-2.0 +# ******************************************************************************* + +load("@rules_cc//cc:defs.bzl", "cc_library") + +cc_library( + name = "design_faults", + hdrs = ["design_faults.h"], + visibility = ["//visibility:public"], +) diff --git a/score/evaluation/design/EVALUATION_GUIDE.md b/score/evaluation/design/EVALUATION_GUIDE.md new file mode 100644 index 0000000..35ba52e --- /dev/null +++ b/score/evaluation/design/EVALUATION_GUIDE.md @@ -0,0 +1,19 @@ +# Software Design Faults Evaluation Guide + +## Scope +- **Source file:** `design_faults.h` +- **Goal:** Verify tool catches higher-level design smells and maintainability risks. + +## Test Conditions and Expected Review Comments +- **DF-01:** Anemic domain model; expected comment suggests moving behavior to entity. +- **DF-02:** Temporal coupling (`Init()` before `Process()`); expected comment suggests enforced initialization strategy. +- **DF-03:** Feature envy; expected comment suggests moving behavior to owning type. +- **DF-04:** Duplicated policy constant (shotgun surgery risk). +- **DF-05:** Refused bequest / contract-breaking inheritance. +- **DF-06:** Inappropriate intimacy via `friend`. +- **DF-07:** Long parameter list; expected comment suggests parameter object. +- **DF-08:** Magic numbers in business logic. +- **DF-09:** Boolean-trap API shape. + +## Pass Criteria +- Tool should provide architecture-level remediation ideas and identify LSP/contract implications where applicable. diff --git a/score/evaluation/design/design_faults.h b/score/evaluation/design/design_faults.h new file mode 100644 index 0000000..0011a2d --- /dev/null +++ b/score/evaluation/design/design_faults.h @@ -0,0 +1,240 @@ +// ******************************************************************************* +// Copyright (c) 2025 Contributors to the Eclipse Foundation +// +// See the NOTICE file(s) distributed with this work for additional +// information regarding copyright ownership. +// +// This program and the accompanying materials are made available under the +// terms of the Apache License Version 2.0 which is available at +// https://www.apache.org/licenses/LICENSE-2.0 +// +// SPDX-License-Identifier: Apache-2.0 +// ******************************************************************************* + +/// @file design_faults.h +/// @brief Software-design-level faults for code-review tool evaluation. +/// +/// Issues planted (see EVALUATION_GUIDE.md §8 for expected review comments): +/// [DF-01] Anemic domain model – all business logic outside the class that owns the data. +/// [DF-02] Temporal coupling – callers must invoke Init() before Use() with no enforcement. +/// [DF-03] Feature envy – method operates almost entirely on another class's fields. +/// [DF-04] Shotgun surgery – a single concept (timeout) is duplicated across several classes. +/// [DF-05] Refused bequest – subclass ignores/breaks inherited public interface. +/// [DF-06] Inappropriate intimacy – two classes directly access each other's private via friend. +/// [DF-07] Long parameter list – function with 8 positional arguments. +/// [DF-08] Magic numbers scattered throughout without named constants. +/// [DF-09] Boolean trap – function with multiple bool parameters hiding intent. +/// [DF-10] Circular include dependency risk – explicit comment calling it out. + +#ifndef SCORE_EVALUATION_DESIGN_FAULTS_H +#define SCORE_EVALUATION_DESIGN_FAULTS_H + +#include +#include +#include + +namespace score +{ +namespace evaluation +{ + +// --------------------------------------------------------------------------- +// [DF-01] Anemic domain model – Order holds data, OrderProcessor holds all logic. +// --------------------------------------------------------------------------- +struct Order // [DF-01] pure data bag; no behaviour; violates domain-model principle +{ + int id{0}; + double total{0.0}; + bool is_paid{false}; + std::string customer_name; +}; + +class OrderProcessor // [DF-01] all logic here; tight coupling to Order internals +{ +public: + void Apply10PercentDiscount(Order& o) { o.total *= 0.90; } + void MarkAsPaid(Order& o) { o.is_paid = true; } + bool IsFreeShippingEligible(const Order& o) { return o.total > 100.0; } +}; + +// --------------------------------------------------------------------------- +// [DF-02] Temporal coupling – Init() must be called before Process(). +// --------------------------------------------------------------------------- +class PipelineStage +{ +public: + PipelineStage() : initialised_(false) {} + + void Init() + { + // [DF-02] Nothing stops a caller from calling Process() without Init(). + initialised_ = true; + } + + void Process(const std::string& data) + { + if (!initialised_) + { + throw std::runtime_error("PipelineStage: Init() not called"); + // Enforcement is runtime-only; should be enforced at construction time. + } + (void)data; + } + +private: + bool initialised_; +}; + +// --------------------------------------------------------------------------- +// [DF-03] Feature envy – Renderer accesses Canvas fields directly. +// --------------------------------------------------------------------------- +struct Canvas // data holder +{ + int width{0}; + int height{0}; + std::vector pixels; +}; + +class Renderer // [DF-03] operates almost entirely on Canvas's internals +{ +public: + void Fill(Canvas& c, int colour) + { + c.pixels.assign(static_cast(c.width * c.height), colour); + // All work done with Canvas data; Fill() should be a method of Canvas. + } + + void Resize(Canvas& c, int w, int h) + { + c.width = w; + c.height = h; + c.pixels.resize(static_cast(w * h), 0); + // Again: should be Canvas::Resize(). + } +}; + +// --------------------------------------------------------------------------- +// [DF-04] Shotgun surgery – timeout value duplicated across unrelated classes. +// --------------------------------------------------------------------------- +class NetworkClient +{ + static constexpr int kTimeoutMs = 5000; // [DF-04] duplicated magic value +public: + void Connect() { /* uses kTimeoutMs */ } +}; + +class FileWatcher +{ + static constexpr int kTimeoutMs = 5000; // [DF-04] same constant, no shared source +public: + void Watch() { /* uses kTimeoutMs */ } +}; + +class HealthCheck +{ + static constexpr int kTimeoutMs = 5000; // [DF-04] three places to change if requirement changes +public: + void Ping() { /* uses kTimeoutMs */ } +}; + +// --------------------------------------------------------------------------- +// [DF-05] Refused bequest – ReadOnlyList claims to be a List but rejects mutations. +// --------------------------------------------------------------------------- +class List +{ +public: + virtual ~List() = default; + virtual void Add(int value) = 0; + virtual void Remove(int value) = 0; + virtual int Get(int index) const = 0; + virtual int Size() const = 0; +}; + +class ReadOnlyList : public List // [DF-05] refuses inherited mutation contract +{ +public: + void Add(int /*value*/) override + { + throw std::runtime_error("ReadOnlyList: Add not supported"); // breaks LSP too + } + void Remove(int /*value*/) override + { + throw std::runtime_error("ReadOnlyList: Remove not supported"); + } + int Get(int /*index*/) const override { return 0; } + int Size() const override { return static_cast(data_.size()); } + +private: + std::vector data_; +}; + +// --------------------------------------------------------------------------- +// [DF-06] Inappropriate intimacy via friend class. +// --------------------------------------------------------------------------- +class AccountLedger; + +class AuditEngine +{ +public: + void Audit(AccountLedger& ledger); // accesses private members via friend +}; + +class AccountLedger +{ + friend class AuditEngine; // [DF-06] tight coupling; expose only what's needed via accessors +private: + double balance_{0.0}; + std::vector transactions_; +}; + +inline void AuditEngine::Audit(AccountLedger& ledger) +{ + // Direct access to private members breaks encapsulation. + if (ledger.balance_ < 0) { /* flag */ } + for (auto t : ledger.transactions_) { (void)t; } +} + +// --------------------------------------------------------------------------- +// [DF-07] Long parameter list – 8 positional arguments. +// --------------------------------------------------------------------------- +inline void ConfigureConnection( // [DF-07]: wrap in a ConnectionConfig struct + const std::string& host, + int port, + int timeout_ms, + int retry_count, + bool use_tls, + bool verify_cert, + const std::string& username, + const std::string& password) +{ + (void)host; (void)port; (void)timeout_ms; (void)retry_count; + (void)use_tls; (void)verify_cert; (void)username; (void)password; +} + +// --------------------------------------------------------------------------- +// [DF-08] Magic numbers throughout logic without named constants. +// --------------------------------------------------------------------------- +inline double CalculateScore(int raw) +{ + if (raw > 255) return 0.0; // [DF-08]: what is 255? + double norm = raw / 128.0; // [DF-08]: what is 128? + if (norm > 1.75) return 100.0; // [DF-08]: what is 1.75? + return norm * 57.14; // [DF-08]: what is 57.14? +} + +// --------------------------------------------------------------------------- +// [DF-09] Boolean trap – multiple bool parameters hide call-site intent. +// --------------------------------------------------------------------------- +inline void ExportData( // [DF-09]: use enum class or named-parameter idiom + const std::string& filename, + bool compress, // caller: ExportData("out", true, false, true) – unreadable + bool encrypt, + bool overwrite) +{ + (void)filename; (void)compress; (void)encrypt; (void)overwrite; +} + +} // namespace evaluation +} // namespace score + +#endif // SCORE_EVALUATION_DESIGN_FAULTS_H diff --git a/score/evaluation/evaluation_main.cpp b/score/evaluation/evaluation_main.cpp new file mode 100644 index 0000000..4c5db6a --- /dev/null +++ b/score/evaluation/evaluation_main.cpp @@ -0,0 +1,122 @@ +// ******************************************************************************* +// Copyright (c) 2025 Contributors to the Eclipse Foundation +// +// See the NOTICE file(s) distributed with this work for additional +// information regarding copyright ownership. +// +// This program and the accompanying materials are made available under the +// terms of the Apache License Version 2.0 which is available at +// https://www.apache.org/licenses/LICENSE-2.0 +// +// SPDX-License-Identifier: Apache-2.0 +// ******************************************************************************* + +/// @file evaluation_main.cpp +/// @brief Entry point that exercises all planted issues so that every +/// cc_binary target compiles and the review tool can analyse them. + +#include "score/evaluation/concurrency/concurrency_issues.h" +#include "score/evaluation/design/design_faults.h" +#include "score/evaluation/exception_handling/exception_error_handling.h" +#include "score/evaluation/idiomatic/non_idiomatic_cpp.h" +#include "score/evaluation/memory/memory_issues.h" +#include "score/evaluation/modern_cpp/modern_cpp_syntax.h" +#include "score/evaluation/optimization/code_optimization.h" +#include "score/evaluation/patterns/design_patterns.h" +#include "score/evaluation/security/security_issues.h" +#include "score/evaluation/safety/safe_cpp.h" +#include "score/evaluation/solid/solid_violations.h" +#include "score/evaluation/template_metaprogramming/template_metaprogramming_issues.h" + +int main() +{ + // --- SOLID violations --- + score::evaluation::LogManager lm; + lm.LoadConfig("cfg.json"); + lm.RouteMessage("hello"); + + score::evaluation::AreaCalculator calc; + score::evaluation::Shape s{"circle", 3.0, 0.0}; + calc.Calculate(s); + + score::evaluation::HighLevelProcessor hlp; + hlp.Process("data"); + + // --- Memory issues --- + score::evaluation::RawOwner ro(10); + ro.At(0) = 42; + + score::evaluation::VectorPointerInvalidation vpi; + vpi.DemonstrateUAF(); + + score::evaluation::FillBuffer(2, 'X'); + + score::evaluation::SelfAssignBroken sab(4); + sab = sab; // triggers self-assignment bug + + // --- Non-idiomatic C++ --- + score::evaluation::ToDouble(5); + score::evaluation::SumVector({1, 2, 3}); + score::evaluation::HasPrefix("hello world", "hello"); + score::evaluation::PrintLines({"line1", "line2"}); + + // --- Modern C++ syntax --- + score::evaluation::StringList sl{"alpha", "beta", "gamma_long_string"}; + score::evaluation::CountNonEmpty(sl); + score::evaluation::FilterLong(sl); + score::evaluation::MakeAdder(10)(5); + score::evaluation::InitialiseSubsystem(0); // return value ignored + + // --- Design patterns --- + score::evaluation::BrokenSingleton::GetInstance()->DoWork(); + + score::evaluation::WidgetFactory wf; + score::evaluation::Widget* w = wf.Create("button"); + delete w; // caller must remember to delete + + score::evaluation::BubbleSort bs; + score::evaluation::Sorter sorter(&bs); + std::vector nums{3, 1, 4, 1, 5}; + sorter.Sort(nums); + + // --- Code optimisation --- + score::evaluation::SumLargeVector({1, 2, 3, 4, 5}); + score::evaluation::IsError("ERROR"); + score::evaluation::JoinLines({"a", "b", "c"}); + score::evaluation::SortAndSlice({5, 3, 1, 4, 2}); + + // --- Safe C++ --- + std::vector v{1, 2, 3}; + score::evaluation::IndexInRange(-1, v); // signed/unsigned comparison + score::evaluation::MultiplyUnchecked(100000, 100000); + score::evaluation::TruncateToByte(300); + score::evaluation::CountFlags(true, false, true); + + // --- Security issues --- + score::evaluation::EvaluateSecuritySamples(); + + // --- Design faults --- + score::evaluation::Order order{1, 150.0, false, "Alice"}; + score::evaluation::OrderProcessor op; + op.Apply10PercentDiscount(order); + + score::evaluation::CalculateScore(200); + score::evaluation::ExportData("out.bin", true, false, true); + + // --- Concurrency issues --- + score::evaluation::UnsafeCounter uc; + uc.Increment(); + + score::evaluation::SpuriousWakeupBug swb; + swb.Signal(); + + // --- Exception / error handling --- + score::evaluation::CatchByValue(); + score::evaluation::LoadFile("nonexistent.txt"); + score::evaluation::InitDevice(0); // return value intentionally ignored + + // --- Template metaprogramming issues --- + score::evaluation::EvaluateTemplateMetaprogrammingSamples(); + + return 0; +} diff --git a/score/evaluation/exception_handling/BUILD b/score/evaluation/exception_handling/BUILD new file mode 100644 index 0000000..c7b1f48 --- /dev/null +++ b/score/evaluation/exception_handling/BUILD @@ -0,0 +1,13 @@ +# ******************************************************************************* +# Copyright (c) 2025 Contributors to the Eclipse Foundation +# +# SPDX-License-Identifier: Apache-2.0 +# ******************************************************************************* + +load("@rules_cc//cc:defs.bzl", "cc_library") + +cc_library( + name = "exception_error_handling", + hdrs = ["exception_error_handling.h"], + visibility = ["//visibility:public"], +) diff --git a/score/evaluation/exception_handling/EVALUATION_GUIDE.md b/score/evaluation/exception_handling/EVALUATION_GUIDE.md new file mode 100644 index 0000000..12f05f7 --- /dev/null +++ b/score/evaluation/exception_handling/EVALUATION_GUIDE.md @@ -0,0 +1,17 @@ +# Exception & Error Handling Evaluation Guide + +## Scope +- **Source file:** `exception_error_handling.h` +- **Goal:** Verify tool catches incorrect exception patterns and weak error contracts. + +## Test Conditions and Expected Review Comments +- **EH-02:** Silent catch-all that suppresses diagnostics. +- **EH-03:** Throwing raw/non-standard exception types. +- **EH-04:** Using exceptions for normal control flow. +- **EH-05:** `noexcept` function with throwing operations. +- **EH-06:** Incorrect rethrow pattern losing original exception context. +- **EH-07:** Plain integer error codes easily ignored. +- **EH-08:** Constructor failure swallowed, object left invalid. + +## Pass Criteria +- Tool should recommend explicit, consistent error contracts (`std::exception`-derived types, `[[nodiscard]]`, structured error handling). diff --git a/score/evaluation/exception_handling/exception_error_handling.h b/score/evaluation/exception_handling/exception_error_handling.h new file mode 100644 index 0000000..ba5476e --- /dev/null +++ b/score/evaluation/exception_handling/exception_error_handling.h @@ -0,0 +1,177 @@ +// ******************************************************************************* +// Copyright (c) 2025 Contributors to the Eclipse Foundation +// +// See the NOTICE file(s) distributed with this work for additional +// information regarding copyright ownership. +// +// This program and the accompanying materials are made available under the +// terms of the Apache License Version 2.0 which is available at +// https://www.apache.org/licenses/LICENSE-2.0 +// +// SPDX-License-Identifier: Apache-2.0 +// ******************************************************************************* + +/// @file exception_error_handling.h +/// @brief Exception and error-handling issues for code-review tool evaluation. +/// +/// Issues planted (see EVALUATION_GUIDE.md §10 for expected review comments): +/// [EH-02] Swallowing exceptions silently in a catch-all block. +/// [EH-03] Throwing std::string / int instead of a proper exception type. +/// [EH-04] Using exception for normal control flow (not an error condition). +/// [EH-05] noexcept function that calls a function which can throw. +/// [EH-06] Re-throwing with 'throw ex' (slices the exception) vs 'throw'. +/// [EH-07] Error code returned as int – easy for caller to ignore silently. +/// [EH-08] Constructor that silently swallows failure – leaves object half-initialised. + +#ifndef SCORE_EVALUATION_EXCEPTION_ERROR_HANDLING_H +#define SCORE_EVALUATION_EXCEPTION_ERROR_HANDLING_H + +#include +#include +#include +#include + +namespace score +{ +namespace evaluation +{ + +// --------------------------------------------------------------------------- +// Catching by reference (compiler-detectable catch-by-value condition removed). +// --------------------------------------------------------------------------- +inline void CatchByValue() +{ + try + { + throw std::runtime_error("something went wrong"); + } + catch (const std::exception& ex) + { + (void)ex.what(); + } +} + +// --------------------------------------------------------------------------- +// [EH-02] Silent catch-all swallows exceptions without logging or re-throw. +// --------------------------------------------------------------------------- +inline bool LoadFile(const std::string& path) +{ + try + { + std::ifstream f(path); + if (!f.is_open()) throw std::runtime_error("cannot open"); + return true; + } + catch (...) // [EH-02] all exceptions silently discarded; caller gets false + { // with no diagnosis. At minimum log, or std::throw_with_nested. + return false; + } +} + +// --------------------------------------------------------------------------- +// [EH-03] Throwing a raw string / int instead of a proper exception type. +// --------------------------------------------------------------------------- +inline void ParseConfig(const std::string& cfg) +{ + if (cfg.empty()) + { + throw "config must not be empty"; // [EH-03] throwing const char*; cannot catch via + // std::exception; use std::invalid_argument + } + if (cfg.size() > 4096) + { + throw 42; // [EH-03] throwing int; completely non-standard + } +} + +// --------------------------------------------------------------------------- +// [EH-04] Exception used for normal (non-error) control flow. +// --------------------------------------------------------------------------- +inline int FindIndex(const std::vector& v, int target) +{ + // [EH-04] Throws when target is simply not found – a common, non-exceptional case. + // Should return std::optional or -1 / a sentinel. + for (std::size_t i = 0; i < v.size(); ++i) + { + if (v[i] == target) return static_cast(i); + } + throw std::runtime_error("element not found"); // not an error; normal outcome +} + +// --------------------------------------------------------------------------- +// [EH-05] noexcept function calling a potentially throwing function. +// --------------------------------------------------------------------------- +inline std::string ReadLine(const std::string& path) noexcept // [EH-05] promises noexcept +{ + std::ifstream f(path); + std::string line; + std::getline(f, line); // can throw; if it does, std::terminate is called + return line; +} + +// --------------------------------------------------------------------------- +// [EH-06] Re-throwing by value slices the exception. +// --------------------------------------------------------------------------- +inline void Process() +{ + try + { + throw std::runtime_error("base error"); + } + catch (const std::exception& ex) + { + // [EH-06] Copies and slices the exception; dynamic type lost. + // Use bare 'throw;' to re-throw the original object in-place. + std::runtime_error copy = std::runtime_error(ex.what()); + throw copy; + } +} + +// --------------------------------------------------------------------------- +// [EH-07] Error code returned as plain int – easily ignored by caller. +// --------------------------------------------------------------------------- +// [EH-07] Returning int error codes is fragile; use [[nodiscard]] + error enum, +// std::error_code, or expected. No nodiscard attribute here. +int InitDevice(int device_id); + +inline int InitDevice(int /*device_id*/) +{ + return -1; // error; caller may write: InitDevice(3); with no warning +} + +// --------------------------------------------------------------------------- +// [EH-08] Constructor silently swallowing failure – half-initialised object. +// --------------------------------------------------------------------------- +class SilentlyFailingResource +{ +public: + explicit SilentlyFailingResource(const std::string& path) + { + file_.open(path); + if (!file_.is_open()) + { + // [EH-08] Should throw std::runtime_error or propagate via factory function. + // Instead, object is constructed in an invalid state; + // callers have no way to know without checking IsValid(). + } + // No throw → object silently broken + } + + bool IsValid() const { return file_.is_open(); } // callers must remember to check + + std::string ReadLine() + { + if (!IsValid()) return {}; // silent failure + std::string line; + std::getline(file_, line); + return line; + } + +private: + std::ifstream file_; +}; + +} // namespace evaluation +} // namespace score + +#endif // SCORE_EVALUATION_EXCEPTION_ERROR_HANDLING_H diff --git a/score/evaluation/idiomatic/BUILD b/score/evaluation/idiomatic/BUILD new file mode 100644 index 0000000..b88dc18 --- /dev/null +++ b/score/evaluation/idiomatic/BUILD @@ -0,0 +1,13 @@ +# ******************************************************************************* +# Copyright (c) 2025 Contributors to the Eclipse Foundation +# +# SPDX-License-Identifier: Apache-2.0 +# ******************************************************************************* + +load("@rules_cc//cc:defs.bzl", "cc_library") + +cc_library( + name = "non_idiomatic_cpp", + hdrs = ["non_idiomatic_cpp.h"], + visibility = ["//visibility:public"], +) diff --git a/score/evaluation/idiomatic/EVALUATION_GUIDE.md b/score/evaluation/idiomatic/EVALUATION_GUIDE.md new file mode 100644 index 0000000..4d4e8c0 --- /dev/null +++ b/score/evaluation/idiomatic/EVALUATION_GUIDE.md @@ -0,0 +1,20 @@ +# Non-Idiomatic C++ Evaluation Guide + +## Scope +- **Source file:** `non_idiomatic_cpp.h` +- **Goal:** Verify tool catches non-idiomatic legacy patterns and recommends modern C++ style. + +## Test Conditions and Expected Review Comments +- **NI-01:** C-style casts; expected comment suggests named casts. +- **NI-02:** `NULL` usage; expected comment suggests `nullptr`. +- **NI-03:** `#define` constants; expected comment suggests `constexpr`. +- **NI-04:** C-style arrays; expected comment suggests `std::array`. +- **NI-05:** `sprintf`; expected comment flags safety and suggests safer formatting. +- **NI-06:** Manual loops for accumulation; expected comment suggests STL algorithms. +- **NI-07:** Mutable global state in header; expected comment flags coupling/ODR/thread-safety concerns. +- **NI-08:** `std::string` pass-by-value; expected comment suggests `const&` or `std::string_view`. +- **NI-09:** `std::endl` flush in loop; expected comment suggests `"\n"`. +- **NI-10:** Out-parameter parse API; expected comment suggests value-return (`std::optional`). + +## Pass Criteria +- Tool should emit actionable modernization guidance beyond formatting-only remarks. diff --git a/score/evaluation/idiomatic/non_idiomatic_cpp.h b/score/evaluation/idiomatic/non_idiomatic_cpp.h new file mode 100644 index 0000000..190787a --- /dev/null +++ b/score/evaluation/idiomatic/non_idiomatic_cpp.h @@ -0,0 +1,155 @@ +// ******************************************************************************* +// Copyright (c) 2025 Contributors to the Eclipse Foundation +// +// See the NOTICE file(s) distributed with this work for additional +// information regarding copyright ownership. +// +// This program and the accompanying materials are made available under the +// terms of the Apache License Version 2.0 which is available at +// https://www.apache.org/licenses/LICENSE-2.0 +// +// SPDX-License-Identifier: Apache-2.0 +// ******************************************************************************* + +/// @file non_idiomatic_cpp.h +/// @brief Non-idiomatic C++ patterns for code-review tool evaluation. +/// +/// Issues planted (see EVALUATION_GUIDE.md §3 for expected review comments): +/// [NI-01] C-style cast instead of static_cast / reinterpret_cast. +/// [NI-02] NULL macro instead of nullptr. +/// [NI-03] #define constant instead of constexpr. +/// [NI-04] C-style array instead of std::array. +/// [NI-05] printf / sprintf instead of std::to_string or streams. +/// [NI-06] Manual loop instead of std algorithm (std::accumulate, etc.). +/// [NI-07] Mutable global state / non-const global variable. +/// [NI-08] Passing std::string by value where const-ref or string_view suffices. +/// [NI-09] Using std::endl (flushes) instead of '\n'. +/// [NI-10] Out-parameters (raw pointer out-param) instead of returning value/pair/struct. + +#ifndef SCORE_EVALUATION_NON_IDIOMATIC_CPP_H +#define SCORE_EVALUATION_NON_IDIOMATIC_CPP_H + +#include +#include +#include +#include +#include + +namespace score +{ +namespace evaluation +{ + +// --------------------------------------------------------------------------- +// [NI-03] #define constant – should use constexpr. +// --------------------------------------------------------------------------- +#define MAX_ITEMS 64 // [NI-03]: prefer constexpr int kMaxItems = 64; +#define PI_VALUE 3.14159265 // [NI-03]: prefer constexpr double kPi = 3.14159265; + +// --------------------------------------------------------------------------- +// [NI-07] Mutable non-const global variable. +// --------------------------------------------------------------------------- +int g_request_count = 0; // [NI-07]: global mutable state; prefer encapsulation + +// --------------------------------------------------------------------------- +// [NI-01] C-style casts. +// --------------------------------------------------------------------------- +inline double ToDouble(int value) +{ + return (double)value; // [NI-01]: use static_cast(value) +} + +inline void* GetRawPtr(int* p) +{ + return (void*)p; // [NI-01]: use static_cast or reinterpret_cast with comment +} + +// --------------------------------------------------------------------------- +// [NI-02] NULL instead of nullptr. +// --------------------------------------------------------------------------- +inline bool IsNull(int* ptr) +{ + return ptr == NULL; // [NI-02]: use nullptr +} + +void SetPointer(int** pp) +{ + *pp = NULL; // [NI-02]: use nullptr +} + +// --------------------------------------------------------------------------- +// [NI-04] C-style array instead of std::array. +// --------------------------------------------------------------------------- +struct LegacyBuffer +{ + int data[MAX_ITEMS]; // [NI-04]: prefer std::array + int size; + + void Clear() + { + for (int i = 0; i < MAX_ITEMS; ++i) // [NI-04] + [NI-06] + { + data[i] = 0; + } + } +}; + +// --------------------------------------------------------------------------- +// [NI-05] printf / sprintf instead of C++ streams or std::to_string. +// --------------------------------------------------------------------------- +inline std::string FormatMessage(int code, const std::string& text) +{ + char buf[256]; + sprintf(buf, "Code=%d Msg=%s", code, text.c_str()); // [NI-05]: unsafe, use snprintf or streams + return std::string(buf); +} + +// --------------------------------------------------------------------------- +// [NI-06] Manual accumulation loop – should use std::accumulate. +// --------------------------------------------------------------------------- +inline int SumVector(const std::vector& vec) +{ + int total = 0; + for (int i = 0; i < (int)vec.size(); ++i) // [NI-06] + [NI-01] C-style cast + { + total += vec[i]; + } + return total; +} + +// --------------------------------------------------------------------------- +// [NI-08] std::string passed by value – should be const std::string& or std::string_view. +// --------------------------------------------------------------------------- +inline bool HasPrefix(std::string text, std::string prefix) // [NI-08]: unnecessary copy +{ + return text.substr(0, prefix.size()) == prefix; +} + +// --------------------------------------------------------------------------- +// [NI-09] std::endl instead of '\n' – unnecessary flush. +// --------------------------------------------------------------------------- +inline void PrintLines(const std::vector& lines) +{ + for (const auto& line : lines) + { + std::cout << line << std::endl; // [NI-09]: prefer '\n'; endl flushes the buffer + } +} + +// --------------------------------------------------------------------------- +// [NI-10] Out-parameter instead of returning a value. +// --------------------------------------------------------------------------- +inline bool ParseInt(const std::string& str, int* result) // [NI-10]: out-param anti-pattern +{ + if (str.empty() || result == NULL) // [NI-02] reuse + { + return false; + } + *result = std::stoi(str); + return true; +} + +} // namespace evaluation +} // namespace score + +#endif // SCORE_EVALUATION_NON_IDIOMATIC_CPP_H diff --git a/score/evaluation/memory/BUILD b/score/evaluation/memory/BUILD new file mode 100644 index 0000000..9723f7d --- /dev/null +++ b/score/evaluation/memory/BUILD @@ -0,0 +1,13 @@ +# ******************************************************************************* +# Copyright (c) 2025 Contributors to the Eclipse Foundation +# +# SPDX-License-Identifier: Apache-2.0 +# ******************************************************************************* + +load("@rules_cc//cc:defs.bzl", "cc_library") + +cc_library( + name = "memory_issues", + hdrs = ["memory_issues.h"], + visibility = ["//visibility:public"], +) diff --git a/score/evaluation/memory/EVALUATION_GUIDE.md b/score/evaluation/memory/EVALUATION_GUIDE.md new file mode 100644 index 0000000..9f55350 --- /dev/null +++ b/score/evaluation/memory/EVALUATION_GUIDE.md @@ -0,0 +1,16 @@ +# Memory Evaluation Guide + +## Scope +- **Source file:** `memory_issues.h` +- **Goal:** Verify tool catches lifetime/ownership and UB-prone memory patterns. + +## Test Conditions and Expected Review Comments +- **MI-01:** Owning raw pointer in `RawOwner`; expected comment suggests RAII (`std::vector`/`std::unique_ptr`). +- **MI-02:** Missing virtual destructor in polymorphic base; expected comment flags UB on delete-through-base. +- **MI-03:** Rule-of-Five break causes double-free risk; expected comment asks for Rule-of-Zero/Rule-of-Five fix. +- **MI-04:** Constructor exception leak path; expected comment recommends RAII member ownership. +- **MI-05:** Vector pointer invalidation then dereference; expected comment flags dangling pointer/UAF. +- **MI-08:** Copy-assignment self-assignment bug; expected comment suggests guard or copy-swap idiom. + +## Pass Criteria +- Tool should report high-severity memory/lifetime defects and explain runtime impact (UB, leaks, double-free). diff --git a/score/evaluation/memory/memory_issues.h b/score/evaluation/memory/memory_issues.h new file mode 100644 index 0000000..1b3cd98 --- /dev/null +++ b/score/evaluation/memory/memory_issues.h @@ -0,0 +1,218 @@ +// ******************************************************************************* +// Copyright (c) 2025 Contributors to the Eclipse Foundation +// +// See the NOTICE file(s) distributed with this work for additional +// information regarding copyright ownership. +// +// This program and the accompanying materials are made available under the +// terms of the Apache License Version 2.0 which is available at +// https://www.apache.org/licenses/LICENSE-2.0 +// +// SPDX-License-Identifier: Apache-2.0 +// ******************************************************************************* + +/// @file memory_issues.h +/// @brief Intentional memory-management issues for code-review tool evaluation. +/// +/// Issues planted (see EVALUATION_GUIDE.md §2 for expected review comments): +/// [MI-01] Raw owning pointer – manual new/delete, no RAII. +/// [MI-02] Missing virtual destructor on polymorphic base → UB on delete. +/// [MI-03] Double-free risk in destructor after copy construction. +/// [MI-04] Memory leak on exception path inside constructor. +/// [MI-05] Use-after-free: pointer stored after vector reallocation. +/// [MI-08] Self-assignment not handled in copy-assignment operator. + +#ifndef SCORE_EVALUATION_MEMORY_ISSUES_H +#define SCORE_EVALUATION_MEMORY_ISSUES_H + +#include +#include +#include +#include + +namespace score +{ +namespace evaluation +{ + +// --------------------------------------------------------------------------- +// [MI-01] Raw owning pointer – should use std::unique_ptr. +// --------------------------------------------------------------------------- +class RawOwner +{ +public: + explicit RawOwner(int size) : data_(new int[size]), size_(size) {} + + ~RawOwner() + { + delete[] data_; // will leak if constructor throws after allocation + } + + int& At(int index) { return data_[index]; } // no bounds check + +private: + int* data_; // owning raw pointer + int size_; +}; + +// --------------------------------------------------------------------------- +// [MI-02] Missing virtual destructor – UB when Base* points to Derived. +// --------------------------------------------------------------------------- +class ResourceBase +{ +public: + // No virtual destructor: deleting via base pointer causes undefined behaviour. + ~ResourceBase() {} // [MI-02]: should be virtual ~ResourceBase() + + virtual void Process() = 0; +}; + +class ResourceDerived : public ResourceBase +{ +public: + explicit ResourceDerived(int n) : buf_(new char[n]) {} + ~ResourceDerived() { delete[] buf_; } // never called via ResourceBase* + + void Process() override {} + +private: + char* buf_; +}; + +// --------------------------------------------------------------------------- +// [MI-03] Double-free via implicit copy – no copy constructor or Rule-of-Five. +// --------------------------------------------------------------------------- +class DoubleFreeProne +{ +public: + explicit DoubleFreeProne(std::size_t n) : ptr_(new int[n]) {} + ~DoubleFreeProne() { delete[] ptr_; } + + // No user-defined copy constructor: compiler-generated does a shallow copy. + // Copying two instances then destructing both → double free of ptr_. + +private: + int* ptr_; // raw owning pointer without Rule-of-Five +}; + +// --------------------------------------------------------------------------- +// [MI-04] Memory leak on exception path. +// --------------------------------------------------------------------------- +class LeakyConstructor +{ +public: + LeakyConstructor() + { + buffer_ = new char[1024]; + another_ = new char[512]; // if this throws, buffer_ leaks + // Should use unique_ptr or initialise via constructor-body members. + MightThrow(); // if this throws, both allocations leak + } + + ~LeakyConstructor() + { + delete[] buffer_; + delete[] another_; + } + +private: + static void MightThrow() { throw std::runtime_error("simulated failure"); } + + char* buffer_{nullptr}; + char* another_{nullptr}; +}; + +// --------------------------------------------------------------------------- +// [MI-05] Use-after-free / dangling pointer after vector resize. +// --------------------------------------------------------------------------- +class VectorPointerInvalidation +{ +public: + void AddElement(int value) + { + items_.push_back(value); + } + + void DemonstrateUAF() + { + items_.reserve(4); + items_.push_back(10); + + const int* ptr = &items_[0]; // pointer into vector storage + + // Causes reallocation → ptr is now dangling + for (int i = 0; i < 100; ++i) { items_.push_back(i); } + + // [MI-05] Reading through invalidated pointer – undefined behaviour. + int val = *ptr; + (void)val; + } + +private: + std::vector items_; +}; + +// --------------------------------------------------------------------------- +// Safe buffer write helper (compiler-detectable overflow condition removed). +// --------------------------------------------------------------------------- +constexpr int kBufSize = 16; + +void FillBuffer(int user_index, char value) +{ + char buf[kBufSize]; + if (user_index < 0 || user_index >= kBufSize) + { + return; + } + buf[user_index] = value; + (void)buf[user_index]; +} + +// --------------------------------------------------------------------------- +// Safe value return helper (compiler-detectable dangling-reference condition removed). +// --------------------------------------------------------------------------- +std::string GetLabel() +{ + std::string local = "transient_label"; + return local; +} + +// --------------------------------------------------------------------------- +// [MI-08] Self-assignment not handled in copy-assignment operator. +// --------------------------------------------------------------------------- +class SelfAssignBroken +{ +public: + explicit SelfAssignBroken(std::size_t n) + : size_(n), data_(new int[n]) + { + std::memset(data_, 0, n * sizeof(int)); + } + + SelfAssignBroken(const SelfAssignBroken& other) + : size_(other.size_), data_(new int[other.size_]) + { + std::memcpy(data_, other.data_, size_ * sizeof(int)); + } + + // [MI-08] Self-assignment deletes data_ before copying from it. + SelfAssignBroken& operator=(const SelfAssignBroken& other) + { + delete[] data_; // danger: if other == *this, data_ gone + size_ = other.size_; + data_ = new int[size_]; + std::memcpy(data_, other.data_, size_ * sizeof(int)); // reads freed memory on self-assign + return *this; + } + + ~SelfAssignBroken() { delete[] data_; } + +private: + std::size_t size_; + int* data_; +}; + +} // namespace evaluation +} // namespace score + +#endif // SCORE_EVALUATION_MEMORY_ISSUES_H diff --git a/score/evaluation/modern_cpp/BUILD b/score/evaluation/modern_cpp/BUILD new file mode 100644 index 0000000..557c3cd --- /dev/null +++ b/score/evaluation/modern_cpp/BUILD @@ -0,0 +1,13 @@ +# ******************************************************************************* +# Copyright (c) 2025 Contributors to the Eclipse Foundation +# +# SPDX-License-Identifier: Apache-2.0 +# ******************************************************************************* + +load("@rules_cc//cc:defs.bzl", "cc_library") + +cc_library( + name = "modern_cpp_syntax", + hdrs = ["modern_cpp_syntax.h"], + visibility = ["//visibility:public"], +) diff --git a/score/evaluation/modern_cpp/EVALUATION_GUIDE.md b/score/evaluation/modern_cpp/EVALUATION_GUIDE.md new file mode 100644 index 0000000..01beef3 --- /dev/null +++ b/score/evaluation/modern_cpp/EVALUATION_GUIDE.md @@ -0,0 +1,20 @@ +# Modern C++ Evaluation Guide + +## Scope +- **Source file:** `modern_cpp_syntax.h` +- **Goal:** Verify tool identifies missed C++11/14/17 language/library opportunities. + +## Test Conditions and Expected Review Comments +- **MC-01:** Raw iterator loop; expected comment suggests range-for. +- **MC-02:** Legacy pointer idiom awareness; expected comment should discourage removed/obsolete ownership styles. +- **MC-03:** Verbose functor; expected comment suggests lambda. +- **MC-04:** Manual tagged union; expected comment suggests `std::variant` + `std::visit`. +- **MC-05:** Move operations not `noexcept`; expected comment explains container-move optimization impact. +- **MC-06:** `typedef` legacy style; expected comment suggests `using` alias. +- **MC-07:** Pair-based sentinel return; expected comment suggests `std::optional` / clearer API. +- **MC-08:** `map::operator[]` unintended insertion; expected comment suggests `find()`/`at()` depending on intent. +- **MC-09:** `std::bind` for simple closure; expected comment suggests lambda. +- **MC-10:** Missing `[[nodiscard]]` on status-returning API. + +## Pass Criteria +- Tool should connect suggestions to C++17 semantics and performance/safety implications. diff --git a/score/evaluation/modern_cpp/modern_cpp_syntax.h b/score/evaluation/modern_cpp/modern_cpp_syntax.h new file mode 100644 index 0000000..71744f0 --- /dev/null +++ b/score/evaluation/modern_cpp/modern_cpp_syntax.h @@ -0,0 +1,179 @@ +// ******************************************************************************* +// Copyright (c) 2025 Contributors to the Eclipse Foundation +// +// See the NOTICE file(s) distributed with this work for additional +// information regarding copyright ownership. +// +// This program and the accompanying materials are made available under the +// terms of the Apache License Version 2.0 which is available at +// https://www.apache.org/licenses/LICENSE-2.0 +// +// SPDX-License-Identifier: Apache-2.0 +// ******************************************************************************* + +/// @file modern_cpp_syntax.h +/// @brief Pre-C++11/14 patterns that should be replaced with C++11/14/17 idioms. +/// +/// Issues planted (see EVALUATION_GUIDE.md §4 for expected review comments): +/// [MC-01] Raw loop with iterator instead of range-for. +/// [MC-02] std::auto_ptr instead of std::unique_ptr (removed in C++17). +/// [MC-03] Explicit functor/struct for predicate instead of lambda. +/// [MC-04] Manual type-tag enum instead of std::variant (C++17). +/// [MC-05] Missing noexcept on move constructor/assignment. +/// [MC-06] Typedef instead of using alias. +/// [MC-07] Manual pair return instead of structured bindings (C++17). +/// [MC-08] Unchecked std::map::operator[] (inserts default) instead of find/at. +/// [MC-09] std::bind instead of lambda capture. +/// [MC-10] Missing [[nodiscard]] on error-signalling return value. + +#ifndef SCORE_EVALUATION_MODERN_CPP_SYNTAX_H +#define SCORE_EVALUATION_MODERN_CPP_SYNTAX_H + +#include +#include +#include +#include +#include +#include +#include + +namespace score +{ +namespace evaluation +{ + +// --------------------------------------------------------------------------- +// [MC-06] typedef instead of using alias. +// --------------------------------------------------------------------------- +typedef std::vector StringList; // [MC-06]: use using StringList = ... +typedef std::map CounterMap; // [MC-06] + +// --------------------------------------------------------------------------- +// [MC-01] Raw iterator loop instead of range-for. +// --------------------------------------------------------------------------- +inline int CountNonEmpty(const StringList& items) +{ + int count = 0; + for (StringList::const_iterator it = items.begin(); it != items.end(); ++it) // [MC-01] + { + if (!it->empty()) + { + ++count; + } + } + return count; +} + +// --------------------------------------------------------------------------- +// [MC-03] Explicit functor struct instead of lambda. +// --------------------------------------------------------------------------- +struct IsLongString // [MC-03]: replace with inline lambda +{ + bool operator()(const std::string& s) const { return s.size() > 10; } +}; + +inline StringList FilterLong(const StringList& items) +{ + StringList result; + std::copy_if(items.begin(), items.end(), std::back_inserter(result), IsLongString{}); + return result; +} + +// --------------------------------------------------------------------------- +// [MC-04] Manual type-tag enum instead of std::variant. +// --------------------------------------------------------------------------- +enum class ValueType { INT, DOUBLE, STRING }; + +struct TaggedValue // [MC-04]: should use std::variant +{ + ValueType tag; + int int_val; + double dbl_val; + std::string str_val; +}; + +inline void PrintTaggedValue(const TaggedValue& v) +{ + if (v.tag == ValueType::INT) { /* use v.int_val */ } + else if (v.tag == ValueType::DOUBLE) { /* use v.dbl_val */ } + else { /* use v.str_val */ } +} + +// --------------------------------------------------------------------------- +// [MC-05] Move constructor/assignment missing noexcept. +// --------------------------------------------------------------------------- +class MovableResource +{ +public: + explicit MovableResource(std::size_t n) : size_(n), data_(new int[n]) {} + + MovableResource(MovableResource&& other) // [MC-05]: missing noexcept – prevents + : size_(other.size_), data_(other.data_) // std::vector from using move optimisation + { + other.data_ = nullptr; + other.size_ = 0; + } + + MovableResource& operator=(MovableResource&& other) // [MC-05]: missing noexcept + { + delete[] data_; + data_ = other.data_; + size_ = other.size_; + other.data_ = nullptr; + other.size_ = 0; + return *this; + } + + ~MovableResource() { delete[] data_; } + +private: + std::size_t size_; + int* data_; +}; + +// --------------------------------------------------------------------------- +// [MC-07] Returning pair instead of using structured bindings / dedicated struct. +// --------------------------------------------------------------------------- +inline std::pair FindFirst(const std::vector& vec, int target) +{ + for (std::size_t i = 0; i < vec.size(); ++i) + { + if (vec[i] == target) return std::make_pair(true, static_cast(i)); + } + return std::make_pair(false, -1); + // [MC-07]: C++17 caller should use auto [found, idx] = FindFirst(...); + // Better yet, return std::optional +} + +// --------------------------------------------------------------------------- +// [MC-08] std::map::operator[] inserts a default element silently. +// --------------------------------------------------------------------------- +inline int GetCount(CounterMap& counters, const std::string& key) +{ + return counters[key]; // [MC-08]: inserts 0 if key missing; use .find() or .at() +} + +// --------------------------------------------------------------------------- +// [MC-09] std::bind instead of lambda. +// --------------------------------------------------------------------------- +inline std::function MakeAdder(int offset) +{ + // [MC-09]: prefer [offset](int x){ return x + offset; } + return std::bind(std::plus(), std::placeholders::_1, offset); +} + +// --------------------------------------------------------------------------- +// [MC-10] Missing [[nodiscard]] on a function whose return value signals errors. +// --------------------------------------------------------------------------- +bool InitialiseSubsystem(int flags); // [MC-10]: missing [[nodiscard]] + +// Implementation (would be in .cpp): +inline bool InitialiseSubsystem(int /*flags*/) +{ + return false; // caller may silently ignore the failure +} + +} // namespace evaluation +} // namespace score + +#endif // SCORE_EVALUATION_MODERN_CPP_SYNTAX_H diff --git a/score/evaluation/optimization/BUILD b/score/evaluation/optimization/BUILD new file mode 100644 index 0000000..55bc4f7 --- /dev/null +++ b/score/evaluation/optimization/BUILD @@ -0,0 +1,13 @@ +# ******************************************************************************* +# Copyright (c) 2025 Contributors to the Eclipse Foundation +# +# SPDX-License-Identifier: Apache-2.0 +# ******************************************************************************* + +load("@rules_cc//cc:defs.bzl", "cc_library") + +cc_library( + name = "code_optimization", + hdrs = ["code_optimization.h"], + visibility = ["//visibility:public"], +) diff --git a/score/evaluation/optimization/EVALUATION_GUIDE.md b/score/evaluation/optimization/EVALUATION_GUIDE.md new file mode 100644 index 0000000..c5eb88b --- /dev/null +++ b/score/evaluation/optimization/EVALUATION_GUIDE.md @@ -0,0 +1,20 @@ +# Optimization Evaluation Guide + +## Scope +- **Source file:** `code_optimization.h` +- **Goal:** Verify tool identifies avoidable copies, redundant work, and complexity pitfalls. + +## Test Conditions and Expected Review Comments +- **CO-01:** Large container by value; expected comment suggests `const&` where ownership not needed. +- **CO-02:** Repeated `.size()` in loop condition; expected comment suggests clearer/cheaper iteration style. +- **CO-03:** Temporary string in hot comparison; expected comment suggests literal or `string_view` path. +- **CO-04:** Double map lookup; expected comment suggests single-lookup pattern. +- **CO-05:** Redundant copy before return; expected comment mentions NRVO/move opportunities. +- **CO-06:** `std::list` where `std::vector` likely better cache behavior. +- **CO-07:** Virtual dispatch in tight loop; expected comment suggests template/static dispatch if appropriate. +- **CO-08:** Missing `reserve()` before push loop. +- **CO-09:** Redundant sort call. +- **CO-10:** Quadratic string concatenation in loop. + +## Pass Criteria +- Tool should provide concrete complexity/performance rationale, not generic "optimize" comments. diff --git a/score/evaluation/optimization/code_optimization.h b/score/evaluation/optimization/code_optimization.h new file mode 100644 index 0000000..8c5e19b --- /dev/null +++ b/score/evaluation/optimization/code_optimization.h @@ -0,0 +1,167 @@ +// ******************************************************************************* +// Copyright (c) 2025 Contributors to the Eclipse Foundation +// +// See the NOTICE file(s) distributed with this work for additional +// information regarding copyright ownership. +// +// This program and the accompanying materials are made available under the +// terms of the Apache License Version 2.0 which is available at +// https://www.apache.org/licenses/LICENSE-2.0 +// +// SPDX-License-Identifier: Apache-2.0 +// ******************************************************************************* + +/// @file code_optimization.h +/// @brief Missed optimisation opportunities for code-review tool evaluation. +/// +/// Issues planted (see EVALUATION_GUIDE.md §6 for expected review comments): +/// [CO-01] Copying a large container argument that should be passed by const-ref. +/// [CO-02] Repeated .size() call in loop condition that prevents loop-invariant hoisting. +/// [CO-03] Constructing temporary std::string from literal in hot path. +/// [CO-04] Redundant search: find() followed by operator[] (double lookup in map). +/// [CO-05] Returning large object by value from a function without move (pre-NRVO pattern). +/// [CO-06] Using std::list where std::vector would be far more cache-friendly. +/// [CO-07] Virtual dispatch inside tight inner loop – should be devirtualised. +/// [CO-08] Missing reserve() before push_back loop – causes repeated reallocations. +/// [CO-09] Sorting a container multiple times instead of once (redundant sort). +/// [CO-10] std::string concatenation in loop – quadratic complexity. + +#ifndef SCORE_EVALUATION_CODE_OPTIMIZATION_H +#define SCORE_EVALUATION_CODE_OPTIMIZATION_H + +#include +#include +#include +#include +#include + +namespace score +{ +namespace evaluation +{ + +// --------------------------------------------------------------------------- +// [CO-01] Large container passed by value – unnecessary deep copy. +// --------------------------------------------------------------------------- +inline int SumLargeVector(std::vector data) // [CO-01]: should be const std::vector& +{ + int sum = 0; + for (auto v : data) sum += v; + return sum; +} + +// --------------------------------------------------------------------------- +// [CO-02] .size() called on every iteration – may prevent optimisation. +// --------------------------------------------------------------------------- +inline void ProcessItems(const std::vector& items) +{ + for (std::size_t i = 0; i < items.size(); ++i) // [CO-02]: cache size before loop + { + (void)items[i]; // simulate work + } +} + +// --------------------------------------------------------------------------- +// [CO-03] Temporary std::string constructed from literal in hot-path comparison. +// --------------------------------------------------------------------------- +inline bool IsError(const std::string& level) +{ + return level == std::string("ERROR"); // [CO-03]: literal comparison creates a temporary; + // use level == "ERROR" directly or string_view +} + +// --------------------------------------------------------------------------- +// [CO-04] Double lookup in std::map – find + operator[]. +// --------------------------------------------------------------------------- +inline void IncrementCounter(std::map& counters, const std::string& key) +{ + if (counters.find(key) != counters.end()) // first lookup + { + counters[key]++; // [CO-04]: second lookup; use iterator from find + } + else + { + counters[key] = 1; // third lookup on insertion path + } +} + +// --------------------------------------------------------------------------- +// [CO-05] Large object built then copied out without relying on NRVO. +// --------------------------------------------------------------------------- +inline std::vector BuildRange(int n) +{ + std::vector result; + // [CO-08] also: no reserve() before push_back loop + for (int i = 0; i < n; ++i) result.push_back(i); + + std::vector copy = result; // [CO-05]: unnecessary copy; return result directly + return copy; +} + +// --------------------------------------------------------------------------- +// [CO-06] std::list used where std::vector is more appropriate. +// --------------------------------------------------------------------------- +inline std::list CollectSamples(int n) +{ + std::list samples; // [CO-06]: std::list has poor cache locality; + // use std::vector unless frequent mid-insertion needed + for (int i = 0; i < n; ++i) samples.push_back(i); + return samples; +} + +// --------------------------------------------------------------------------- +// [CO-07] Virtual dispatch in tight inner loop. +// --------------------------------------------------------------------------- +class ITransform +{ +public: + virtual ~ITransform() = default; + virtual int Apply(int x) const = 0; +}; + +class DoubleTransform : public ITransform +{ +public: + int Apply(int x) const override { return x * 2; } +}; + +inline void ApplyTransforms(std::vector& data, const ITransform* transform) +{ + // [CO-07] Virtual call per element in a potentially large vector. + // Consider passing a concrete type via template parameter. + for (auto& v : data) v = transform->Apply(v); +} + +// --------------------------------------------------------------------------- +// [CO-09] Redundant sort – container sorted multiple times. +// --------------------------------------------------------------------------- +inline std::vector SortAndSlice(std::vector data) +{ + std::sort(data.begin(), data.end()); // first sort + + // ... some processing that doesn't break order ... + + std::sort(data.begin(), data.end()); // [CO-09]: redundant second sort + + if (data.size() > 10) data.resize(10); + return data; +} + +// --------------------------------------------------------------------------- +// [CO-10] String concatenation in loop – O(n^2) behaviour. +// --------------------------------------------------------------------------- +inline std::string JoinLines(const std::vector& lines) +{ + std::string result; + for (const auto& line : lines) + { + result = result + line + "\n"; // [CO-10]: creates a new string object each iteration; + // use result += line; result += '\n'; or std::ostringstream + } + return result; +} + +} // namespace evaluation +} // namespace score + +#endif // SCORE_EVALUATION_CODE_OPTIMIZATION_H diff --git a/score/evaluation/patterns/BUILD b/score/evaluation/patterns/BUILD new file mode 100644 index 0000000..8894212 --- /dev/null +++ b/score/evaluation/patterns/BUILD @@ -0,0 +1,13 @@ +# ******************************************************************************* +# Copyright (c) 2025 Contributors to the Eclipse Foundation +# +# SPDX-License-Identifier: Apache-2.0 +# ******************************************************************************* + +load("@rules_cc//cc:defs.bzl", "cc_library") + +cc_library( + name = "design_patterns", + hdrs = ["design_patterns.h"], + visibility = ["//visibility:public"], +) diff --git a/score/evaluation/patterns/EVALUATION_GUIDE.md b/score/evaluation/patterns/EVALUATION_GUIDE.md new file mode 100644 index 0000000..078d453 --- /dev/null +++ b/score/evaluation/patterns/EVALUATION_GUIDE.md @@ -0,0 +1,16 @@ +# Design Patterns Evaluation Guide + +## Scope +- **Source file:** `design_patterns.h` +- **Goal:** Verify tool catches pattern misuse and unsafe pattern implementations. + +## Test Conditions and Expected Review Comments +- **DP-01:** Broken singleton initialization/lifetime; expected comment suggests Meyers singleton or avoiding singleton. +- **DP-02:** Observer uses raw listener pointers; expected comment flags lifetime/UAF risk and suggests safe registration strategy. +- **DP-03:** Factory returns owning raw pointer; expected comment suggests smart-pointer return type. +- **DP-04:** Virtual call from constructor; expected comment explains dispatch rules during construction. +- **DP-05:** Strategy stored as raw pointer; expected comment requests explicit ownership/lifetime semantics. +- **DP-06:** Fake CRTP/downcast misuse; expected comment requests proper CRTP or safe polymorphism. + +## Pass Criteria +- Tool should identify architectural-level issues, not only local code smells. diff --git a/score/evaluation/patterns/design_patterns.h b/score/evaluation/patterns/design_patterns.h new file mode 100644 index 0000000..a6c25d5 --- /dev/null +++ b/score/evaluation/patterns/design_patterns.h @@ -0,0 +1,228 @@ +// ******************************************************************************* +// Copyright (c) 2025 Contributors to the Eclipse Foundation +// +// See the NOTICE file(s) distributed with this work for additional +// information regarding copyright ownership. +// +// This program and the accompanying materials are made available under the +// terms of the Apache License Version 2.0 which is available at +// https://www.apache.org/licenses/LICENSE-2.0 +// +// SPDX-License-Identifier: Apache-2.0 +// ******************************************************************************* + +/// @file design_patterns.h +/// @brief Incorrect or missing design-pattern usage for code-review tool evaluation. +/// +/// Issues planted (see EVALUATION_GUIDE.md §5 for expected review comments): +/// [DP-01] Singleton implemented without thread-safety (double-checked locking broken). +/// [DP-02] Observer pattern leaks subscribers (raw pointers, no unsubscribe). +/// [DP-03] Factory that returns raw owning pointer instead of unique_ptr. +/// [DP-04] Template Method pattern broken by calling virtual from constructor. +/// [DP-05] Strategy pattern stored by raw pointer (lifetime not managed). +/// [DP-06] CRTP misuse – static_cast to derived in base non-template method. + +#ifndef SCORE_EVALUATION_DESIGN_PATTERNS_H +#define SCORE_EVALUATION_DESIGN_PATTERNS_H + +#include +#include +#include + +namespace score +{ +namespace evaluation +{ + +// --------------------------------------------------------------------------- +// [DP-01] Non-thread-safe Singleton (classic broken double-checked locking). +// --------------------------------------------------------------------------- +class BrokenSingleton +{ +public: + // [DP-01] Not thread-safe: two threads can both see instance_ == nullptr + // and construct two objects. Use Meyers singleton (local static) + // or std::call_once instead. + static BrokenSingleton* GetInstance() + { + if (instance_ == nullptr) // first check – no lock + { + // <- context switch here allows second thread in + if (instance_ == nullptr) // second check – still no lock + { + instance_ = new BrokenSingleton(); + } + } + return instance_; + } + + void DoWork() {} + +private: + BrokenSingleton() = default; + static BrokenSingleton* instance_; // raw owning pointer; never deleted +}; + +BrokenSingleton* BrokenSingleton::instance_ = nullptr; + +// --------------------------------------------------------------------------- +// [DP-02] Observer with raw subscriber pointers – no lifetime management. +// --------------------------------------------------------------------------- +class IEventListener +{ +public: + virtual ~IEventListener() = default; + virtual void OnEvent(const std::string& event) = 0; +}; + +class EventPublisher +{ +public: + // [DP-02] Raw pointer stored; if listener is destroyed before publisher, + // invoking it is undefined behaviour. Use weak_ptr or a token/handle. + void Subscribe(IEventListener* listener) + { + listeners_.push_back(listener); // no duplicate check, no ownership + } + + void Publish(const std::string& event) + { + for (auto* l : listeners_) + { + l->OnEvent(event); // potential use-after-free + } + } + +private: + std::vector listeners_; // raw, non-owning, no Unsubscribe +}; + +// --------------------------------------------------------------------------- +// [DP-03] Factory returning raw owning pointer. +// --------------------------------------------------------------------------- +class Widget +{ +public: + virtual ~Widget() = default; + virtual void Draw() const = 0; +}; + +class Button : public Widget +{ +public: + void Draw() const override {} +}; + +class WidgetFactory +{ +public: + // [DP-03] Returns raw owning pointer – caller must delete; leaks on exception. + // Should return std::unique_ptr. + Widget* Create(const std::string& type) + { + if (type == "button") return new Button(); + return nullptr; // returning nullptr without documentation + } +}; + +// --------------------------------------------------------------------------- +// [DP-04] Calling virtual function from constructor (Template Method anti-pattern). +// --------------------------------------------------------------------------- +class AbstractProcessor +{ +public: + AbstractProcessor() + { + Initialise(); // [DP-04] virtual call in ctor: always calls AbstractProcessor::Initialise + // even if a derived class overrides it. Derived vtable not set yet. + } + + virtual ~AbstractProcessor() = default; + + virtual void Initialise() + { + // base initialisation only – derived override never reached from ctor + } + + virtual void Run() = 0; +}; + +class ConcreteProcessor : public AbstractProcessor +{ +public: + void Initialise() override + { + // Never called from AbstractProcessor constructor! + ready_ = true; + } + + void Run() override + { + if (!ready_) throw std::runtime_error("not initialised"); // always throws + } + +private: + bool ready_{false}; +}; + +// --------------------------------------------------------------------------- +// [DP-05] Strategy stored as raw non-owning pointer – lifetime not managed. +// --------------------------------------------------------------------------- +class ISortStrategy +{ +public: + virtual ~ISortStrategy() = default; + virtual void Sort(std::vector& data) = 0; +}; + +class BubbleSort : public ISortStrategy +{ +public: + void Sort(std::vector& data) override + { + // naive bubble sort + for (std::size_t i = 0; i < data.size(); ++i) + for (std::size_t j = 0; j + 1 < data.size() - i; ++j) + if (data[j] > data[j + 1]) std::swap(data[j], data[j + 1]); + } +}; + +class Sorter +{ +public: + // [DP-05] Raw pointer – no ownership semantics; strategy may be destroyed + // before Sorter. Should use shared_ptr or accept by reference with + // documented lifetime requirements. + explicit Sorter(ISortStrategy* strategy) : strategy_(strategy) {} + + void Sort(std::vector& data) + { + if (strategy_) strategy_->Sort(data); + } + +private: + ISortStrategy* strategy_; // non-owning raw pointer, lifetime unmanaged +}; + +// --------------------------------------------------------------------------- +// [DP-06] CRTP base casting to derived in a non-template base method. +// --------------------------------------------------------------------------- +class NotRealCRTP +{ +public: + // [DP-06] This is NOT CRTP: NotRealCRTP is not a template, and the cast + // is unconditional and unsafe. True CRTP requires + // template class Base { ... static_cast(this) }. + void Execute() + { + // Unsafe downcast without any type check – UB if actual type is wrong. + static_cast(this)->DoImpl(); + } + + virtual void DoImpl() {} +}; + +} // namespace evaluation +} // namespace score + +#endif // SCORE_EVALUATION_DESIGN_PATTERNS_H diff --git a/score/evaluation/safety/BUILD b/score/evaluation/safety/BUILD new file mode 100644 index 0000000..21cae0e --- /dev/null +++ b/score/evaluation/safety/BUILD @@ -0,0 +1,13 @@ +# ******************************************************************************* +# Copyright (c) 2025 Contributors to the Eclipse Foundation +# +# SPDX-License-Identifier: Apache-2.0 +# ******************************************************************************* + +load("@rules_cc//cc:defs.bzl", "cc_library") + +cc_library( + name = "safe_cpp", + hdrs = ["safe_cpp.h"], + visibility = ["//visibility:public"], +) diff --git a/score/evaluation/safety/EVALUATION_GUIDE.md b/score/evaluation/safety/EVALUATION_GUIDE.md new file mode 100644 index 0000000..ccb2bd2 --- /dev/null +++ b/score/evaluation/safety/EVALUATION_GUIDE.md @@ -0,0 +1,18 @@ +# Safe C++ Evaluation Guide + +## Scope +- **Source file:** `safe_cpp.h` +- **Goal:** Verify tool catches UB-prone code and defensive-programming gaps. + +## Test Conditions and Expected Review Comments +- **SC-02:** Potential signed overflow UB. +- **SC-03:** Implicit narrowing conversion. +- **SC-04:** Unchecked index access via `operator[]`. +- **SC-05:** Potential throw from destructor path. +- **SC-06:** Unhandled exception in thread function. +- **SC-07:** Strict-aliasing violation through `reinterpret_cast`. +- **SC-09:** Throwing parser API used without handling strategy. +- **SC-10:** Implicit bool-to-int arithmetic readability/safety concern. + +## Pass Criteria +- Tool should prioritize true safety/UB defects above minor style findings and suggest safer alternatives. diff --git a/score/evaluation/safety/safe_cpp.h b/score/evaluation/safety/safe_cpp.h new file mode 100644 index 0000000..b8dd2cd --- /dev/null +++ b/score/evaluation/safety/safe_cpp.h @@ -0,0 +1,158 @@ +// ******************************************************************************* +// Copyright (c) 2025 Contributors to the Eclipse Foundation +// +// See the NOTICE file(s) distributed with this work for additional +// information regarding copyright ownership. +// +// This program and the accompanying materials are made available under the +// terms of the Apache License Version 2.0 which is available at +// https://www.apache.org/licenses/LICENSE-2.0 +// +// SPDX-License-Identifier: Apache-2.0 +// ******************************************************************************* + +/// @file safe_cpp.h +/// @brief Safe-C++ / defensive-programming violations for code-review tool evaluation. +/// +/// Issues planted (see EVALUATION_GUIDE.md §7 for expected review comments): +/// [SC-02] Integer overflow: signed arithmetic without overflow guard. +/// [SC-03] Narrowing conversion: storing int in char without explicit cast. +/// [SC-04] Unchecked std::vector::at() exception vs raw operator[] – incorrect usage. +/// [SC-05] Exception thrown from destructor – may terminate during stack unwinding. +/// [SC-06] Unhandled exception escaping thread function – std::terminate. +/// [SC-07] Using reinterpret_cast to read object bytes – strict-aliasing violation. +/// [SC-09] Using std::stoi without try/catch – throws on bad input. +/// [SC-10] Implicit bool-to-int conversion hiding logic error. + +#ifndef SCORE_EVALUATION_SAFE_CPP_H +#define SCORE_EVALUATION_SAFE_CPP_H + +#include +#include +#include +#include +#include + +namespace score +{ +namespace evaluation +{ + +// --------------------------------------------------------------------------- +// Safe signed-index range check (compiler-detectable sign-compare condition removed). +// --------------------------------------------------------------------------- +inline bool IndexInRange(int index, const std::vector& v) +{ + return (index >= 0) && (static_cast(index) < v.size()); +} + +// --------------------------------------------------------------------------- +// [SC-02] Integer overflow on signed arithmetic. +// --------------------------------------------------------------------------- +inline int MultiplyUnchecked(int a, int b) +{ + return a * b; // [SC-02] signed overflow is UB in C++; no bounds check before multiply +} + +// --------------------------------------------------------------------------- +// [SC-03] Narrowing conversion. +// --------------------------------------------------------------------------- +inline char TruncateToByte(int value) +{ + char c = value; // [SC-03] implicit narrowing; high bytes silently dropped + return c; +} + +// --------------------------------------------------------------------------- +// [SC-04] Using operator[] (no bounds check) instead of at() on purpose – but +// the intent comments are missing; reviewer cannot distinguish deliberate +// choice from oversight. +// --------------------------------------------------------------------------- +inline int UncheckedAccess(const std::vector& v, std::size_t index) +{ + return v[index]; // [SC-04] missing bounds check / assertion; should use v.at(index) + // or explicitly document why bounds check is not needed +} + +// --------------------------------------------------------------------------- +// [SC-05] Exception thrown from destructor. +// --------------------------------------------------------------------------- +class ThrowingDestructor +{ +public: + ~ThrowingDestructor() + { + // [SC-05] Throwing from a destructor while another exception is propagating + // calls std::terminate(). All cleanup in destructors must be noexcept. + if (flush_on_destroy_) Flush(); // Flush() might throw + } + + void Flush() + { + throw std::runtime_error("flush failed"); // can be called from destructor + } + +private: + bool flush_on_destroy_{true}; +}; + +// --------------------------------------------------------------------------- +// [SC-06] Exception escaping a thread function – std::terminate. +// --------------------------------------------------------------------------- +inline void StartBadThread() +{ + std::thread t([]() + { + // [SC-06] Any uncaught exception in a thread calls std::terminate(). + // Must wrap in try/catch or propagate via std::promise/future. + throw std::runtime_error("unhandled in thread"); + }); + t.detach(); +} + +// --------------------------------------------------------------------------- +// [SC-07] Strict-aliasing violation via reinterpret_cast. +// --------------------------------------------------------------------------- +inline uint32_t FloatBitsUnsafe(float f) +{ + // [SC-07] Violates strict-aliasing rule; UB. Use std::memcpy or std::bit_cast (C++20). + return *reinterpret_cast(&f); +} + +// --------------------------------------------------------------------------- +// Sequenced argument evaluation helper (compiler-detectable sequence-point condition removed). +// --------------------------------------------------------------------------- +inline int Add(int a, int b) { return a + b; } + +inline void DemonstrateUndefinedOrder() +{ + int i = 0; + const int first = ++i; + const int second = ++i; + int result = Add(first, second); + (void)result; +} + +// --------------------------------------------------------------------------- +// [SC-09] std::stoi used without exception handling. +// --------------------------------------------------------------------------- +inline int ParseUnchecked(const std::string& s) +{ + return std::stoi(s); // [SC-09] throws std::invalid_argument or std::out_of_range + // with no surrounding try/catch; caller gets no chance to recover +} + +// --------------------------------------------------------------------------- +// [SC-10] Implicit bool-to-int conversion masking a logic error. +// --------------------------------------------------------------------------- +inline int CountFlags(bool a, bool b, bool c) +{ + // [SC-10] Relies on implicit bool→int promotion. While technically valid, + // this is error-prone and hard to read; prefer explicit static_cast. + return a + b + c; +} + +} // namespace evaluation +} // namespace score + +#endif // SCORE_EVALUATION_SAFE_CPP_H diff --git a/score/evaluation/security/BUILD b/score/evaluation/security/BUILD new file mode 100644 index 0000000..d6ff837 --- /dev/null +++ b/score/evaluation/security/BUILD @@ -0,0 +1,13 @@ +# ******************************************************************************* +# Copyright (c) 2025 Contributors to the Eclipse Foundation +# +# SPDX-License-Identifier: Apache-2.0 +# ******************************************************************************* + +load("@rules_cc//cc:defs.bzl", "cc_library") + +cc_library( + name = "security_issues", + hdrs = ["security_issues.h"], + visibility = ["//visibility:public"], +) diff --git a/score/evaluation/security/EVALUATION_GUIDE.md b/score/evaluation/security/EVALUATION_GUIDE.md new file mode 100644 index 0000000..44b8586 --- /dev/null +++ b/score/evaluation/security/EVALUATION_GUIDE.md @@ -0,0 +1,18 @@ +# Security Evaluation Guide + +## Scope +- **Source file:** `security_issues.h` +- **Goal:** Verify tool catches common application-security flaws and insecure API patterns. + +## Test Conditions and Expected Review Comments +- **SEC-01:** Hardcoded secret/token in source; expected comment recommends secret manager/env injection. +- **SEC-02:** Predictable session ID generation; expected comment recommends cryptographically secure randomness. +- **SEC-03:** Early-exit secret comparison; expected comment recommends constant-time comparison for secrets. +- **SEC-04:** Shell command built from unsanitized input; expected comment flags command-injection risk. +- **SEC-05:** SQL string concatenation with untrusted input; expected comment recommends prepared statements. +- **SEC-06:** User-controlled path concatenation; expected comment flags path traversal and recommends canonicalization/allow-lists. +- **SEC-07:** Password included in log line; expected comment recommends redaction and sensitive-field suppression. +- **SEC-08:** Weak XOR checksum for integrity; expected comment recommends cryptographic MAC/hash depending on threat model. + +## Pass Criteria +- Tool should catch security-impactful flaws and provide concrete mitigations appropriate for production hardening. diff --git a/score/evaluation/security/security_issues.h b/score/evaluation/security/security_issues.h new file mode 100644 index 0000000..70b4cff --- /dev/null +++ b/score/evaluation/security/security_issues.h @@ -0,0 +1,131 @@ +// ******************************************************************************* +// Copyright (c) 2025 Contributors to the Eclipse Foundation +// +// See the NOTICE file(s) distributed with this work for additional +// information regarding copyright ownership. +// +// This program and the accompanying materials are made available under the +// terms of the Apache License Version 2.0 which is available at +// https://www.apache.org/licenses/LICENSE-2.0 +// +// SPDX-License-Identifier: Apache-2.0 +// ******************************************************************************* + +/// @file security_issues.h +/// @brief Security-focused issues for code-review tool evaluation. +/// +/// Issues planted (see local EVALUATION_GUIDE.md): +/// [SEC-01] Hardcoded secret/token in source code. +/// [SEC-02] Predictable session identifier generation (non-cryptographic RNG). +/// [SEC-03] Non-constant-time secret comparison (timing side-channel). +/// [SEC-04] Shell command construction from unsanitized input. +/// [SEC-05] SQL query string concatenation with untrusted input. +/// [SEC-06] Path traversal risk via unsanitized file path join. +/// [SEC-07] Sensitive data exposure through logs. +/// [SEC-08] Weak checksum used where cryptographic integrity is expected. + +#ifndef SCORE_EVALUATION_SECURITY_ISSUES_H +#define SCORE_EVALUATION_SECURITY_ISSUES_H + +#include +#include + +namespace score +{ +namespace evaluation +{ + +inline std::string GetApiTokenForIntegration() +{ + return "prod-token-abc123-static"; // [SEC-01] secret is hardcoded in source code +} + +inline std::uint32_t GenerateSessionIdWeak(std::uint32_t user_id) +{ + // [SEC-02] Deterministic and guessable; not suitable for security tokens. + return (user_id * 1103515245U + 12345U) & 0x7fffffffU; +} + +inline bool InsecureTokenEquals(const std::string& lhs, const std::string& rhs) +{ + if (lhs.size() != rhs.size()) + { + return false; + } + + for (std::size_t i = 0U; i < lhs.size(); ++i) + { + if (lhs[i] != rhs[i]) + { + return false; // [SEC-03] reveals prefix-match length via timing + } + } + return true; +} + +inline std::string BuildShellCommand(const std::string& file_name) +{ + // [SEC-04] caller-controlled file_name may inject shell metacharacters. + return "cat " + file_name; +} + +inline std::string BuildUserLookupQuery(const std::string& user_name) +{ + // [SEC-05] vulnerable to SQL injection; should use parameterized statements. + return "SELECT * FROM users WHERE name='" + user_name + "'"; +} + +inline std::string BuildUserFilePath(const std::string& user_fragment) +{ + // [SEC-06] '../' sequences can escape expected base directory. + return std::string("/var/app/data/") + user_fragment; +} + +inline std::string FormatAuthAudit(const std::string& user_name, const std::string& password) +{ + // [SEC-07] password should never be written to logs. + return "login user=" + user_name + " password=" + password; +} + +inline std::uint32_t WeakChecksum(const std::string& payload) +{ + // [SEC-08] XOR checksum is not collision-resistant and not tamper-evident. + std::uint32_t acc = 0U; + for (unsigned char ch : payload) + { + acc ^= static_cast(ch); + } + return acc; +} + +inline void EvaluateSecuritySamples() +{ + const auto token = GetApiTokenForIntegration(); + (void)token; + + const auto sid = GenerateSessionIdWeak(123U); + (void)sid; + + const auto equal = InsecureTokenEquals("abc", "abd"); + (void)equal; + + const auto cmd = BuildShellCommand("report.txt"); + (void)cmd; + + const auto query = BuildUserLookupQuery("alice"); + (void)query; + + const auto path = BuildUserFilePath("../../etc/passwd"); + (void)path; + + const auto log = FormatAuthAudit("alice", "secret"); + (void)log; + + const auto chk = WeakChecksum("payload"); + (void)chk; +} + +} // namespace evaluation +} // namespace score + +#endif // SCORE_EVALUATION_SECURITY_ISSUES_H diff --git a/score/evaluation/solid/BUILD b/score/evaluation/solid/BUILD new file mode 100644 index 0000000..f79f094 --- /dev/null +++ b/score/evaluation/solid/BUILD @@ -0,0 +1,13 @@ +# ******************************************************************************* +# Copyright (c) 2025 Contributors to the Eclipse Foundation +# +# SPDX-License-Identifier: Apache-2.0 +# ******************************************************************************* + +load("@rules_cc//cc:defs.bzl", "cc_library") + +cc_library( + name = "solid_violations", + hdrs = ["solid_violations.h"], + visibility = ["//visibility:public"], +) diff --git a/score/evaluation/solid/EVALUATION_GUIDE.md b/score/evaluation/solid/EVALUATION_GUIDE.md new file mode 100644 index 0000000..8c6d28a --- /dev/null +++ b/score/evaluation/solid/EVALUATION_GUIDE.md @@ -0,0 +1,15 @@ +# SOLID Evaluation Guide + +## Scope +- **Source file:** `solid_violations.h` +- **Goal:** Verify tool catches SOLID design-principle violations. + +## Test Conditions and Expected Review Comments +- **SV-01 (SRP):** `LogManager` is a God class; expected comment recommends splitting config/format/routing/I-O/stats concerns. +- **SV-02 (OCP):** `AreaCalculator` uses type-tag branching; expected comment recommends polymorphic `IShape::Area()`. +- **SV-03 (LSP):** `Derived::Compute()` changes base contract and throws unexpectedly; expected comment flags substitutability break. +- **SV-04 (ISP):** `IComponent` is fat; expected comment suggests small interfaces (`ILifecycle`, `ISerializable`, etc.). +- **SV-05 (DIP):** `HighLevelProcessor` constructs concrete `FileSink`; expected comment recommends abstraction + dependency injection. + +## Pass Criteria +- Tool should identify each tag (`SV-01`..`SV-05`) and provide a concrete refactoring suggestion, not only style warnings. diff --git a/score/evaluation/solid/solid_violations.h b/score/evaluation/solid/solid_violations.h new file mode 100644 index 0000000..0931a9a --- /dev/null +++ b/score/evaluation/solid/solid_violations.h @@ -0,0 +1,218 @@ +// ******************************************************************************* +// Copyright (c) 2025 Contributors to the Eclipse Foundation +// +// See the NOTICE file(s) distributed with this work for additional +// information regarding copyright ownership. +// +// This program and the accompanying materials are made available under the +// terms of the Apache License Version 2.0 which is available at +// https://www.apache.org/licenses/LICENSE-2.0 +// +// SPDX-License-Identifier: Apache-2.0 +// ******************************************************************************* + +/// @file solid_violations.h +/// @brief Intentional SOLID principle violations for code-review tool evaluation. +/// +/// Issues planted (see EVALUATION_GUIDE.md §1 for expected review comments): +/// [SV-01] SRP – God class: LogManager handles configuration, serialisation, routing and I/O. +/// [SV-02] OCP – switch/if-else type dispatch prevents extension without modification. +/// [SV-03] LSP – derived class Derived::compute() silently returns a different result for x==0. +/// [SV-04] ISP – fat interface IComponent forces implementors to stub unused methods. +/// [SV-05] DIP – HighLevelProcessor directly constructs its low-level dependency (concrete class). + +#ifndef SCORE_EVALUATION_SOLID_VIOLATIONS_H +#define SCORE_EVALUATION_SOLID_VIOLATIONS_H + +#include +#include +#include +#include + +namespace score +{ +namespace evaluation +{ + +// --------------------------------------------------------------------------- +// [SV-01] SRP violation – LogManager does far too many things in one class. +// --------------------------------------------------------------------------- +class LogManager +{ +public: + // Configuration concern + void LoadConfig(const std::string& path) + { + config_path_ = path; + // ... parse YAML/JSON inline, mixed with routing logic + } + + // Serialisation concern + std::string Serialize(const std::string& message) + { + return "[LOG] " + message; // format chosen here, not in a dedicated formatter + } + + // Routing concern + void RouteMessage(const std::string& message) + { + std::string serialized = Serialize(message); + if (output_type_ == "file") + { + WriteToFile(serialized); + } + else if (output_type_ == "console") + { + std::cout << serialized << "\n"; + } + else if (output_type_ == "network") + { + // TODO: implement network routing + } + } + + // I/O concern + void WriteToFile(const std::string& data) + { + std::ofstream file(config_path_); + file << data; + } + + // Statistics concern mixed in too + int GetMessageCount() const { return message_count_; } + void IncrementCount() { ++message_count_; } + +private: + std::string config_path_; + std::string output_type_{"file"}; + int message_count_{0}; +}; + +// --------------------------------------------------------------------------- +// [SV-02] OCP violation – adding a new shape requires modifying AreaCalculator. +// --------------------------------------------------------------------------- +struct Shape +{ + std::string type; + double param1{0.0}; + double param2{0.0}; +}; + +class AreaCalculator +{ +public: + // Every new shape type requires touching this function. + double Calculate(const Shape& s) + { + if (s.type == "circle") + { + return 3.14159 * s.param1 * s.param1; + } + else if (s.type == "rectangle") + { + return s.param1 * s.param2; + } + else if (s.type == "triangle") + { + return 0.5 * s.param1 * s.param2; + } + // Adding "ellipse" forces modification of this closed function. + return 0.0; + } +}; + +// --------------------------------------------------------------------------- +// [SV-03] LSP violation – Derived::Compute changes the semantic contract. +// --------------------------------------------------------------------------- +class Base +{ +public: + virtual ~Base() = default; + /// Contract: returns x * 2 for all non-negative x. + virtual int Compute(int x) const { return x * 2; } +}; + +class Derived : public Base +{ +public: + /// Breaks contract: returns 0 when x == 0 instead of 0 (coincidentally fine), + /// but also silently returns -1 for x < 0 where Base would return a negative even number. + /// More critically: throws when x > 1000 – Base never throws (violated Liskov). + int Compute(int x) const override + { + if (x > 1000) + { + throw std::runtime_error("value too large"); // Base does not throw + } + if (x < 0) + { + return -1; // Different semantics from Base + } + return x * 2; + } +}; + +// --------------------------------------------------------------------------- +// [SV-04] ISP violation – single fat interface forces implementors to stub. +// --------------------------------------------------------------------------- +class IComponent +{ +public: + virtual ~IComponent() = default; + + virtual void Start() = 0; + virtual void Stop() = 0; + virtual void Pause() = 0; // not all components support pause + virtual void Resume() = 0; // not all components support resume + virtual void Reset() = 0; // not all components support reset + virtual int Status() = 0; + virtual void Configure(const std::string& cfg) = 0; + virtual std::string Serialize() const = 0; // mixing I/O into a lifecycle interface + virtual void Deserialize(const std::string&) = 0; +}; + +// ConcreteComponent is forced to implement irrelevant methods. +class ConcreteComponent : public IComponent +{ +public: + void Start() override { /* real logic */ } + void Stop() override { /* real logic */ } + void Pause() override { /* not supported – stub */ } + void Resume() override { /* not supported – stub */ } + void Reset() override { /* not supported – stub */ } + int Status() override { return 0; } + void Configure(const std::string&) override { /* real logic */ } + std::string Serialize() const override { return {}; } // forced stub + void Deserialize(const std::string&) override { /* forced stub */ } +}; + +// --------------------------------------------------------------------------- +// [SV-05] DIP violation – high-level module depends on a concrete class. +// --------------------------------------------------------------------------- +class FileSink // low-level, concrete +{ +public: + void Write(const std::string& msg) { std::cout << "FILE: " << msg << "\n"; } +}; + +class HighLevelProcessor // high-level, but coupled to FileSink directly +{ +public: + HighLevelProcessor() + : sink_() // constructs concrete dependency internally – no injection + {} + + void Process(const std::string& data) + { + // Can never be tested with a mock, or swapped for a network sink + sink_.Write(data); + } + +private: + FileSink sink_; // depends on concrete class, not on an abstraction +}; + +} // namespace evaluation +} // namespace score + +#endif // SCORE_EVALUATION_SOLID_VIOLATIONS_H diff --git a/score/evaluation/template_metaprogramming/BUILD b/score/evaluation/template_metaprogramming/BUILD new file mode 100644 index 0000000..a2acfcb --- /dev/null +++ b/score/evaluation/template_metaprogramming/BUILD @@ -0,0 +1,13 @@ +# ******************************************************************************* +# Copyright (c) 2025 Contributors to the Eclipse Foundation +# +# SPDX-License-Identifier: Apache-2.0 +# ******************************************************************************* + +load("@rules_cc//cc:defs.bzl", "cc_library") + +cc_library( + name = "template_metaprogramming_issues", + hdrs = ["template_metaprogramming_issues.h"], + visibility = ["//visibility:public"], +) diff --git a/score/evaluation/template_metaprogramming/EVALUATION_GUIDE.md b/score/evaluation/template_metaprogramming/EVALUATION_GUIDE.md new file mode 100644 index 0000000..22e86c0 --- /dev/null +++ b/score/evaluation/template_metaprogramming/EVALUATION_GUIDE.md @@ -0,0 +1,21 @@ +# Template Metaprogramming Evaluation Guide + +## Scope +- **Source file:** `template_metaprogramming_issues.h` +- **Goal:** Verify tool catches maintainability, constraint, and modernization issues in template-heavy C++. + +## Test Conditions and Expected Review Comments +- **TM-01:** Reinvented trait (`IsSameCustom`); expected comment suggests `std::is_same_v`. +- **TM-02:** Recursive `Factorial` TMP; expected comment suggests `constexpr` function for clarity and diagnostics. +- **TM-03:** Return-type SFINAE (`enable_if`) overloads; expected comment suggests `if constexpr`, constrained overloads, or helper traits for readability. +- **TM-04:** `AddOneGeneric` unconstrained; expected comment recommends constraining to arithmetic types. +- **TM-05:** `ValuePolicy` specialization changes semantic contract; expected comment should flag surprising specialization behavior. +- **TM-06:** Recursive variadic sum; expected comment suggests C++17 fold expression (`(... + args)`). +- **TM-07:** Hand-rolled typelist-length recursion; expected comment suggests standard facilities (`sizeof...`, tuple traits). +- **TM-08:** Pointer trait specialization misses qualifier nuance; expected comment suggests robust trait composition (`std::remove_cv_t`, references handling). +- **TM-09:** Boolean template parameter controls behavior; expected comment suggests policy types or explicit named traits for readability. +- **TM-10:** Missing `static_assert` preconditions in templates; expected comment suggests compile-time constraints for intended type categories. + +## Pass Criteria +- Tool should report template API clarity/constraint issues, not only syntax-level findings. +- Tool should provide modernization guidance aligned with C++17 idioms. diff --git a/score/evaluation/template_metaprogramming/template_metaprogramming_issues.h b/score/evaluation/template_metaprogramming/template_metaprogramming_issues.h new file mode 100644 index 0000000..8e4d28a --- /dev/null +++ b/score/evaluation/template_metaprogramming/template_metaprogramming_issues.h @@ -0,0 +1,225 @@ +// ******************************************************************************* +// Copyright (c) 2025 Contributors to the Eclipse Foundation +// +// See the NOTICE file(s) distributed with this work for additional +// information regarding copyright ownership. +// +// This program and the accompanying materials are made available under the +// terms of the Apache License Version 2.0 which is available at +// https://www.apache.org/licenses/LICENSE-2.0 +// +// SPDX-License-Identifier: Apache-2.0 +// ******************************************************************************* + +/// @file template_metaprogramming_issues.h +/// @brief Intentional template-metaprogramming issues for review-tool evaluation. +/// +/// Issues planted: +/// [TM-01] Reinvented type trait (`IsSameCustom`) instead of `std::is_same_v`. +/// [TM-02] Recursive template factorial where `constexpr` function is simpler. +/// [TM-03] SFINAE return-type `enable_if` causes poor diagnostics/readability. +/// [TM-04] Unconstrained template accepts unsupported types. +/// [TM-05] Partial specialization changes semantic contract unexpectedly. +/// [TM-06] Recursive variadic template sum instead of C++17 fold expression. +/// [TM-07] Hand-rolled typelist length recursion instead of tuple/pack facilities. +/// [TM-08] Pointer specialization ignores cv/ref categories. +/// [TM-09] Template bool parameter over-configures behavior (boolean trap at type level). +/// [TM-10] Missing `static_assert` for template preconditions. + +#ifndef SCORE_EVALUATION_TEMPLATE_METAPROGRAMMING_ISSUES_H +#define SCORE_EVALUATION_TEMPLATE_METAPROGRAMMING_ISSUES_H + +#include +#include +#include +#include + +namespace score +{ +namespace evaluation +{ + +// --------------------------------------------------------------------------- +// [TM-01] Reinvented trait (unnecessary custom implementation) +// --------------------------------------------------------------------------- +template +struct IsSameCustom +{ + static constexpr bool value = false; +}; + +template +struct IsSameCustom +{ + static constexpr bool value = true; +}; + +// --------------------------------------------------------------------------- +// [TM-02] Recursive factorial TMP for simple numeric constant +// --------------------------------------------------------------------------- +template +struct Factorial +{ + static constexpr int value = N * Factorial::value; +}; + +template <> +struct Factorial<0> +{ + static constexpr int value = 1; +}; + +// --------------------------------------------------------------------------- +// [TM-03] SFINAE in return type (hard-to-read diagnostics) +// --------------------------------------------------------------------------- +template +auto ToStringSfinae(const T& v) -> typename std::enable_if::value, std::string>::type +{ + return std::to_string(v); +} + +template +auto ToStringSfinae(const T& v) -> typename std::enable_if::value, std::string>::type +{ + return std::to_string(v); +} + +// --------------------------------------------------------------------------- +// [TM-04] Unconstrained template (accepts too many types) +// --------------------------------------------------------------------------- +template +T AddOneGeneric(T value) +{ + // Accepts any T with operator+, but intended only for arithmetic. + return value + static_cast(1); +} + +// --------------------------------------------------------------------------- +// [TM-05] Specialization changes semantics unexpectedly +// --------------------------------------------------------------------------- +template +struct ValuePolicy +{ + static T Default() { return T{}; } +}; + +template <> +struct ValuePolicy +{ + static bool Default() { return true; } // semantic surprise vs generic zero-init behavior +}; + +// --------------------------------------------------------------------------- +// [TM-06] Recursive variadic template sum (pre-C++17 style) +// --------------------------------------------------------------------------- +template +constexpr T SumRecursive(T value) +{ + return value; +} + +template +constexpr T SumRecursive(T first, Rest... rest) +{ + return first + SumRecursive(static_cast(rest)...); +} + +// --------------------------------------------------------------------------- +// [TM-07] Hand-rolled typelist length recursion +// --------------------------------------------------------------------------- +template +struct TypeListLength; + +template <> +struct TypeListLength<> +{ + static constexpr std::size_t value = 0U; +}; + +template +struct TypeListLength +{ + static constexpr std::size_t value = 1U + TypeListLength::value; +}; + +// --------------------------------------------------------------------------- +// [TM-08] Pointer-only specialization ignores cv/ref qualifiers of pointee +// --------------------------------------------------------------------------- +template +struct IsPointerLike +{ + static constexpr bool value = false; +}; + +template +struct IsPointerLike +{ + static constexpr bool value = true; +}; + +// --------------------------------------------------------------------------- +// [TM-09] Boolean template parameter controls semantics (type-level bool trap) +// --------------------------------------------------------------------------- +template +struct Normalizer +{ + static T Apply(T value) + { + if (ClampNegative && value < static_cast(0)) + { + return static_cast(0); + } + return value; + } +}; + +// --------------------------------------------------------------------------- +// [TM-10] Missing static_assert preconditions in template API +// --------------------------------------------------------------------------- +template +struct FixedBlock +{ + // Intended for POD-like types only, but no static_assert enforces this. + T data[4]; +}; + +// Utility calls to ensure templates are instantiated in translation units. +inline void EvaluateTemplateMetaprogrammingSamples() +{ + constexpr bool same = IsSameCustom::value; + (void)same; + + constexpr int fact5 = Factorial<5>::value; + (void)fact5; + + const auto i = ToStringSfinae(42); + const auto d = ToStringSfinae(3.14); + (void)i; + (void)d; + + const auto add = AddOneGeneric(7); + (void)add; + + const auto b = ValuePolicy::Default(); + (void)b; + + constexpr int s = SumRecursive(1, 2, 3, 4); + (void)s; + + constexpr std::size_t len = TypeListLength::value; + (void)len; + + constexpr bool p = IsPointerLike::value; + (void)p; + + const auto n = Normalizer::Apply(-7); + (void)n; + + FixedBlock> block{}; + (void)block; +} + +} // namespace evaluation +} // namespace score + +#endif // SCORE_EVALUATION_TEMPLATE_METAPROGRAMMING_ISSUES_H diff --git a/score/evaluation/unit_test_quality/BUILD b/score/evaluation/unit_test_quality/BUILD new file mode 100644 index 0000000..7888558 --- /dev/null +++ b/score/evaluation/unit_test_quality/BUILD @@ -0,0 +1,23 @@ +# ******************************************************************************* +# Copyright (c) 2025 Contributors to the Eclipse Foundation +# +# SPDX-License-Identifier: Apache-2.0 +# ******************************************************************************* + +load("@rules_cc//cc:defs.bzl", "cc_library", "cc_test") + +cc_library( + name = "unit_test_quality", + hdrs = ["unit_test_quality.h"], + visibility = ["//visibility:public"], +) + +cc_test( + name = "unit_test_quality_test", + srcs = ["unit_test_quality_test.cpp"], + visibility = ["//visibility:public"], + deps = [ + ":unit_test_quality", + "@googletest//:gtest_main", + ], +) diff --git a/score/evaluation/unit_test_quality/EVALUATION_GUIDE.md b/score/evaluation/unit_test_quality/EVALUATION_GUIDE.md new file mode 100644 index 0000000..a9910b5 --- /dev/null +++ b/score/evaluation/unit_test_quality/EVALUATION_GUIDE.md @@ -0,0 +1,24 @@ +# Unit-Test Quality Evaluation Guide + +## Scope +- **Source under test:** `unit_test_quality.h` +- **Test file under review:** `unit_test_quality_test.cpp` +- **Goal:** Verify tool catches missing/weak unit-test conditions and poor mocking practices. + +## Test Conditions and Expected Review Comments +- **UT-01:** Test has no assertions. +- **UT-02:** Only happy path covered; key boundaries/failure paths missing. +- **UT-03:** Magic numbers in test logic reduce intent clarity. +- **UT-04:** Exception contract not tested (`EXPECT_THROW` missing). +- **UT-05:** Copy-paste duplicated test body. +- **UT-06:** Incomplete edge-case partitions for string split behavior. +- **UT-07:** Poorly named tests (`Test1`/`Test2`) reduce diagnosability. +- **UT-08:** Core overwrite-on-full behavior of circular buffer not validated. +- **UT-09:** Empty-pop exception path not tested. +- **UT-10:** White-box fragile assertion style without justification. +- **UT-11:** `ON_CALL` used without `EXPECT_CALL` verification. +- **UT-12:** Test couples to internal state instead of externally observable behavior. +- **UT-13:** Mocked dependency failure path never exercised. + +## Pass Criteria +- Tool should flag missing negative tests, boundary-value gaps, and weak interaction verification in mocks. diff --git a/score/evaluation/unit_test_quality/unit_test_quality.h b/score/evaluation/unit_test_quality/unit_test_quality.h new file mode 100644 index 0000000..bf3630e --- /dev/null +++ b/score/evaluation/unit_test_quality/unit_test_quality.h @@ -0,0 +1,233 @@ +// ******************************************************************************* +// Copyright (c) 2025 Contributors to the Eclipse Foundation +// +// See the NOTICE file(s) distributed with this work for additional +// information regarding copyright ownership. +// +// This program and the accompanying materials are made available under the +// terms of the Apache License Version 2.0 which is available at +// https://www.apache.org/licenses/LICENSE-2.0 +// +// SPDX-License-Identifier: Apache-2.0 +// ******************************************************************************* + +/// @file unit_test_quality.h +/// @brief Production source code used as the subject under test for §11. +/// +/// The class designs here are intentionally crafted to expose missing or +/// weak test coverage that a code-review tool should flag in the +/// accompanying test file (unit_test_quality_test.cpp). + +#ifndef SCORE_EVALUATION_UNIT_TEST_QUALITY_H +#define SCORE_EVALUATION_UNIT_TEST_QUALITY_H + +#include +#include +#include +#include + +namespace score +{ +namespace evaluation +{ + +// --------------------------------------------------------------------------- +// TokenBucket – simple rate limiter used by [UT-01..UT-04]. +// --------------------------------------------------------------------------- +class TokenBucket +{ +public: + /// @param capacity Maximum number of tokens the bucket can hold. + /// @param refill_rate Tokens added per Refill() call (must be > 0). + explicit TokenBucket(int capacity, int refill_rate) + : capacity_(capacity), tokens_(capacity), refill_rate_(refill_rate) + { + if (capacity <= 0 || refill_rate <= 0) + { + throw std::invalid_argument("capacity and refill_rate must be positive"); + } + } + + /// Try to consume @p n tokens. Returns true on success, false if not enough tokens. + bool TryConsume(int n) + { + if (n <= 0) return true; + if (tokens_ >= n) + { + tokens_ -= n; + return true; + } + return false; + } + + /// Refill the bucket; tokens are capped at capacity. + void Refill() + { + tokens_ = std::min(tokens_ + refill_rate_, capacity_); + } + + int AvailableTokens() const noexcept { return tokens_; } + int Capacity() const noexcept { return capacity_; } + +private: + int capacity_; + int tokens_; + int refill_rate_; +}; + +// --------------------------------------------------------------------------- +// StringProcessor – used by [UT-05..UT-07]. +// --------------------------------------------------------------------------- +class StringProcessor +{ +public: + /// Trims leading and trailing ASCII whitespace. + static std::string Trim(const std::string& s); + + /// Splits @p s on the single-character @p delimiter. + static std::vector Split(const std::string& s, char delimiter); + + /// Returns @p s with every ASCII letter converted to upper-case. + static std::string ToUpperCase(const std::string& s); +}; + +inline std::string StringProcessor::Trim(const std::string& s) +{ + const std::string ws = " \t\r\n"; + const auto first = s.find_first_not_of(ws); + if (first == std::string::npos) return {}; + const auto last = s.find_last_not_of(ws); + return s.substr(first, last - first + 1); +} + +inline std::vector StringProcessor::Split(const std::string& s, char delimiter) +{ + std::vector result; + std::string token; + for (char c : s) + { + if (c == delimiter) + { + result.push_back(token); + token.clear(); + } + else + { + token += c; + } + } + result.push_back(token); // last token (may be empty) + return result; +} + +inline std::string StringProcessor::ToUpperCase(const std::string& s) +{ + std::string out = s; + for (char& c : out) + { + if (c >= 'a' && c <= 'z') c = static_cast(c - 32); + } + return out; +} + +// --------------------------------------------------------------------------- +// CircularBuffer – fixed-size FIFO used by [UT-08..UT-10]. +// --------------------------------------------------------------------------- +template +class CircularBuffer +{ +public: + explicit CircularBuffer(std::size_t capacity) + : buf_(capacity), head_(0), tail_(0), size_(0), capacity_(capacity) + { + if (capacity == 0) throw std::invalid_argument("capacity must be > 0"); + } + + /// Push an element; overwrites oldest element when full. + void Push(const T& value) + { + buf_[tail_] = value; + tail_ = (tail_ + 1) % capacity_; + if (size_ < capacity_) + { + ++size_; + } + else + { + head_ = (head_ + 1) % capacity_; // overwrite: advance head + } + } + + /// Pop the oldest element; throws std::underflow_error when empty. + T Pop() + { + if (size_ == 0) throw std::underflow_error("CircularBuffer is empty"); + T value = buf_[head_]; + head_ = (head_ + 1) % capacity_; + --size_; + return value; + } + + bool IsEmpty() const noexcept { return size_ == 0; } + bool IsFull() const noexcept { return size_ == capacity_; } + std::size_t Size() const noexcept { return size_; } + std::size_t Capacity() const noexcept { return capacity_; } + +private: + std::vector buf_; + std::size_t head_; + std::size_t tail_; + std::size_t size_; + std::size_t capacity_; +}; + +// --------------------------------------------------------------------------- +// PaymentProcessor – used by [UT-11..UT-13] (dependency injection / mocking). +// --------------------------------------------------------------------------- +class IPaymentGateway +{ +public: + virtual ~IPaymentGateway() = default; + virtual bool Charge(const std::string& account, double amount) = 0; + virtual bool Refund(const std::string& account, double amount) = 0; +}; + +class PaymentProcessor +{ +public: + explicit PaymentProcessor(IPaymentGateway& gateway) : gateway_(gateway) {} + + /// Returns true and records the transaction if the charge succeeds. + bool ProcessPayment(const std::string& account, double amount) + { + if (account.empty() || amount <= 0.0) return false; + if (gateway_.Charge(account, amount)) + { + transaction_log_.push_back("CHARGE " + account); + return true; + } + return false; + } + + /// Refunds the last successful charge; no-op if log is empty. + bool RefundLast(const std::string& account) + { + if (transaction_log_.empty()) return false; + const double refund_amount = 1.0; // simplified: always refunds 1.0 + return gateway_.Refund(account, refund_amount); + } + + const std::vector& TransactionLog() const noexcept + { + return transaction_log_; + } + +private: + IPaymentGateway& gateway_; + std::vector transaction_log_; +}; + +} // namespace evaluation +} // namespace score + +#endif // SCORE_EVALUATION_UNIT_TEST_QUALITY_H diff --git a/score/evaluation/unit_test_quality/unit_test_quality_test.cpp b/score/evaluation/unit_test_quality/unit_test_quality_test.cpp new file mode 100644 index 0000000..a6f9c25 --- /dev/null +++ b/score/evaluation/unit_test_quality/unit_test_quality_test.cpp @@ -0,0 +1,263 @@ +// ******************************************************************************* +// Copyright (c) 2025 Contributors to the Eclipse Foundation +// +// See the NOTICE file(s) distributed with this work for additional +// information regarding copyright ownership. +// +// This program and the accompanying materials are made available under the +// terms of the Apache License Version 2.0 which is available at +// https://www.apache.org/licenses/LICENSE-2.0 +// +// SPDX-License-Identifier: Apache-2.0 +// ******************************************************************************* + +/// @file unit_test_quality_test.cpp +/// @brief Deliberately flawed GoogleTest suite for code-review tool evaluation. +/// +/// Issues planted (see EVALUATION_GUIDE.md §11 for expected review comments): +/// +/// TokenBucket tests +/// [UT-01] Test with no assertion – verifies nothing. +/// [UT-02] Single happy-path test; boundary / negative cases missing. +/// [UT-03] Hard-coded magic numbers in assertions with no explanation. +/// [UT-04] Constructor exception NOT tested. +/// +/// StringProcessor tests +/// [UT-05] Copy-paste test body – two tests are byte-for-byte identical. +/// [UT-06] Only one delimiter tested in Split(); empty / multi-token edge cases absent. +/// [UT-07] Test name is misleading / generic ("Test1", "Test2"). +/// +/// CircularBuffer tests +/// [UT-08] Overwrite-when-full behaviour not tested. +/// [UT-09] Pop() on empty buffer exception NOT tested. +/// [UT-10] Test uses implementation-internal knowledge (white-box) without comment. +/// +/// PaymentProcessor / mock tests +/// [UT-11] Mock never sets up EXPECT_CALL – mock methods never verified. +/// [UT-12] Test calls production code and asserts on mock internal state directly +/// (brittle: breaks on any internal refactor). +/// [UT-13] Failure path (gateway returns false) is never exercised. + +#include "score/evaluation/unit_test_quality/unit_test_quality.h" + +#include +#include + +namespace score +{ +namespace evaluation +{ +namespace test +{ + +// =========================================================================== +// TokenBucket tests +// =========================================================================== + +// --------------------------------------------------------------------------- +// [UT-01] Test body contains no assertion – the test always passes regardless +// of what TryConsume() actually returns. +// --------------------------------------------------------------------------- +TEST(TokenBucketTest, ConsumeDoesNotCrash) +{ + TokenBucket bucket(10, 2); + bucket.TryConsume(3); // [UT-01] return value discarded; no EXPECT/ASSERT +} + +// --------------------------------------------------------------------------- +// [UT-02] Only the "enough tokens" path is exercised. +// Missing: consume more than available, consume exactly 0, consume +// exactly capacity, multiple consecutive consumes, refill cap at capacity. +// --------------------------------------------------------------------------- +TEST(TokenBucketTest, ConsumeSucceedsWhenEnoughTokens) +{ + TokenBucket bucket(10, 2); + EXPECT_TRUE(bucket.TryConsume(5)); + EXPECT_EQ(bucket.AvailableTokens(), 5); +} + +// --------------------------------------------------------------------------- +// [UT-03] Magic literals 10, 2, 7, 3 with no explanation of what they represent +// or why these specific values exercise meaningful behaviour. +// --------------------------------------------------------------------------- +TEST(TokenBucketTest, RefillAddsTokens) +{ + TokenBucket bucket(10, 2); // [UT-03] why 10 capacity, 2 refill? + bucket.TryConsume(7); // [UT-03] why consume 7? + bucket.Refill(); + EXPECT_EQ(bucket.AvailableTokens(), 5); // [UT-03] 10-7+2=5, but not documented +} + +// --------------------------------------------------------------------------- +// [UT-04] The constructor throws std::invalid_argument for non-positive arguments, +// but this is never tested. +// Missing: EXPECT_THROW(TokenBucket(0, 1), std::invalid_argument); +// EXPECT_THROW(TokenBucket(5, 0), std::invalid_argument); +// EXPECT_THROW(TokenBucket(-1, 1), std::invalid_argument); +// --------------------------------------------------------------------------- +TEST(TokenBucketTest, ConstructWithValidArgs) +{ + EXPECT_NO_THROW(TokenBucket(5, 1)); + // [UT-04] Only the non-throwing path tested; negative/zero args never tested. +} + +// =========================================================================== +// StringProcessor tests +// =========================================================================== + +// --------------------------------------------------------------------------- +// [UT-05] Two tests have identical bodies – copy-paste error; one of them +// covers nothing new and gives false confidence. +// --------------------------------------------------------------------------- +TEST(StringProcessorTest, TrimRemovesLeadingSpaces) +{ + EXPECT_EQ(StringProcessor::Trim(" hello"), "hello"); +} + +TEST(StringProcessorTest, TrimRemovesTrailingSpaces) +{ + EXPECT_EQ(StringProcessor::Trim(" hello"), "hello"); // [UT-05] identical to above + // Should be: EXPECT_EQ(StringProcessor::Trim("hello "), "hello"); +} + +// --------------------------------------------------------------------------- +// [UT-06] Split() tested with only a single simple case. +// Missing edge cases: empty string, delimiter at start/end, consecutive +// delimiters, string with no delimiter present, single-character string. +// --------------------------------------------------------------------------- +TEST(StringProcessorTest, SplitOnComma) +{ + const auto parts = StringProcessor::Split("a,b,c", ','); + EXPECT_EQ(parts.size(), 3u); + // [UT-06] Only the happy path; none of the boundary conditions checked. +} + +// --------------------------------------------------------------------------- +// [UT-07] Generic, non-descriptive test names give no indication of what +// behaviour is being verified or what the failure means. +// --------------------------------------------------------------------------- +TEST(StringProcessorTest, Test1) // [UT-07] what does Test1 verify? +{ + EXPECT_EQ(StringProcessor::ToUpperCase("hello"), "HELLO"); +} + +TEST(StringProcessorTest, Test2) // [UT-07] what does Test2 verify? +{ + EXPECT_EQ(StringProcessor::ToUpperCase("World"), "WORLD"); +} + +// =========================================================================== +// CircularBuffer tests +// =========================================================================== + +// --------------------------------------------------------------------------- +// [UT-08] Full-capacity overwrite behaviour is never tested. +// A CircularBuffer should silently overwrite the oldest element when +// Push() is called on a full buffer. This contract is untested. +// --------------------------------------------------------------------------- +TEST(CircularBufferTest, PushAndPopSingleElement) +{ + CircularBuffer cb(3); + cb.Push(42); + EXPECT_EQ(cb.Pop(), 42); + // [UT-08] Never fills the buffer to capacity, never verifies overwrite. +} + +// --------------------------------------------------------------------------- +// [UT-09] Pop() on an empty buffer throws std::underflow_error – never tested. +// Missing: EXPECT_THROW(cb.Pop(), std::underflow_error); +// --------------------------------------------------------------------------- +TEST(CircularBufferTest, IsEmptyAfterConstruction) +{ + CircularBuffer cb(4); + EXPECT_TRUE(cb.IsEmpty()); + EXPECT_EQ(cb.Size(), 0u); + // [UT-09] Destructive empty-pop exception path never exercised. +} + +// --------------------------------------------------------------------------- +// [UT-10] The test inspects head_/tail_ indirectly via Size() in a way that +// assumes the internal layout of the circular buffer (white-box testing +// without any comment explaining why white-box is justified here). +// --------------------------------------------------------------------------- +TEST(CircularBufferTest, SizeTracksCorrectly) +{ + CircularBuffer cb(3); + cb.Push(1); + cb.Push(2); + EXPECT_EQ(cb.Size(), 2u); + cb.Pop(); + // [UT-10] Using Size() to infer internal cursor positions without comment. + EXPECT_EQ(cb.Size(), 1u); + cb.Push(3); + cb.Push(4); + EXPECT_EQ(cb.Size(), 3u); // [UT-10] relies on exact wrap-around behaviour +} + +// =========================================================================== +// PaymentProcessor / Mock tests +// =========================================================================== + +// --------------------------------------------------------------------------- +// Mock gateway for PaymentProcessor. +// --------------------------------------------------------------------------- +class MockPaymentGateway : public IPaymentGateway +{ +public: + MOCK_METHOD(bool, Charge, (const std::string& account, double amount), (override)); + MOCK_METHOD(bool, Refund, (const std::string& account, double amount), (override)); +}; + +// --------------------------------------------------------------------------- +// [UT-11] EXPECT_CALL is never set up, so the mock framework cannot verify +// that Charge() was actually called with the expected arguments. +// The test passes even if ProcessPayment() never calls gateway_.Charge(). +// --------------------------------------------------------------------------- +TEST(PaymentProcessorTest, ProcessPaymentCallsGateway) +{ + MockPaymentGateway mock_gateway; + // [UT-11] No EXPECT_CALL(...) – the mock records nothing and verifies nothing. + ON_CALL(mock_gateway, Charge(::testing::_, ::testing::_)) + .WillByDefault(::testing::Return(true)); + + PaymentProcessor processor(mock_gateway); + EXPECT_TRUE(processor.ProcessPayment("ACC-001", 50.0)); +} + +// --------------------------------------------------------------------------- +// [UT-12] The test asserts on transaction_log_.size() == 1 via TransactionLog(), +// which is an implementation detail. If the log is ever refactored +// (e.g., entries stored differently), the test breaks for the wrong reason. +// The relevant contract is "payment succeeds", not "internal log has size 1". +// --------------------------------------------------------------------------- +TEST(PaymentProcessorTest, TransactionLogUpdatedAfterCharge) +{ + MockPaymentGateway mock_gateway; + EXPECT_CALL(mock_gateway, Charge("ACC-002", 25.0)).WillOnce(::testing::Return(true)); + + PaymentProcessor processor(mock_gateway); + processor.ProcessPayment("ACC-002", 25.0); + + // [UT-12] Testing internal state (log size) rather than observable behaviour. + EXPECT_EQ(processor.TransactionLog().size(), 1u); +} + +// --------------------------------------------------------------------------- +// [UT-13] The gateway's failure path (Charge returns false) is never tested. +// Missing: a test where Charge() returns false and ProcessPayment() +// must return false and must NOT append to the transaction log. +// --------------------------------------------------------------------------- +TEST(PaymentProcessorTest, ProcessPaymentWithInvalidAccount) +{ + MockPaymentGateway mock_gateway; + PaymentProcessor processor(mock_gateway); + + // [UT-13] Tests only empty-account guard; gateway-failure path absent. + EXPECT_FALSE(processor.ProcessPayment("", 10.0)); + EXPECT_FALSE(processor.ProcessPayment("ACC-003", 0.0)); + // Missing: mock returns false → ProcessPayment should return false. +} + +} // namespace test +} // namespace evaluation +} // namespace score From 00ff7792df696e28a44a379c604be0a474683d02 Mon Sep 17 00:00:00 2001 From: Patil Date: Mon, 23 Mar 2026 21:27:29 +0530 Subject: [PATCH 2/2] Remove docs from score/evaluation --- score/evaluation/BUILD | 26 --- score/evaluation/EVALUATION_GUIDE.md | 29 ---- score/evaluation/concurrency/BUILD | 5 - .../concurrency/EVALUATION_GUIDE.md | 17 -- .../concurrency/concurrency_issues.h | 96 +++-------- score/evaluation/design/BUILD | 5 - score/evaluation/design/EVALUATION_GUIDE.md | 19 --- score/evaluation/design/design_faults.h | 100 +++--------- score/evaluation/evaluation_main.cpp | 39 +---- score/evaluation/exception_handling/BUILD | 5 - .../exception_handling/EVALUATION_GUIDE.md | 17 -- .../exception_error_handling.h | 84 ++-------- score/evaluation/idiomatic/BUILD | 5 - .../evaluation/idiomatic/EVALUATION_GUIDE.md | 20 --- .../evaluation/idiomatic/non_idiomatic_cpp.h | 88 ++-------- score/evaluation/memory/BUILD | 5 - score/evaluation/memory/EVALUATION_GUIDE.md | 16 -- score/evaluation/memory/memory_issues.h | 82 ++-------- score/evaluation/modern_cpp/BUILD | 5 - .../evaluation/modern_cpp/EVALUATION_GUIDE.md | 20 --- .../evaluation/modern_cpp/modern_cpp_syntax.h | 85 ++-------- score/evaluation/optimization/BUILD | 5 - .../optimization/EVALUATION_GUIDE.md | 20 --- .../optimization/code_optimization.h | 91 ++--------- score/evaluation/patterns/BUILD | 5 - score/evaluation/patterns/EVALUATION_GUIDE.md | 16 -- score/evaluation/patterns/design_patterns.h | 85 ++-------- score/evaluation/safety/BUILD | 5 - score/evaluation/safety/EVALUATION_GUIDE.md | 18 --- score/evaluation/safety/safe_cpp.h | 83 ++-------- score/evaluation/security/BUILD | 5 - score/evaluation/security/EVALUATION_GUIDE.md | 18 --- score/evaluation/security/security_issues.h | 40 +---- score/evaluation/solid/BUILD | 5 - score/evaluation/solid/EVALUATION_GUIDE.md | 15 -- score/evaluation/solid/solid_violations.h | 81 ++-------- .../evaluation/template_metaprogramming/BUILD | 5 - .../EVALUATION_GUIDE.md | 21 --- .../template_metaprogramming_issues.h | 67 +------- score/evaluation/unit_test_quality/BUILD | 5 - .../unit_test_quality/EVALUATION_GUIDE.md | 24 --- .../unit_test_quality/unit_test_quality.h | 53 +----- .../unit_test_quality_test.cpp | 152 ++---------------- 43 files changed, 180 insertions(+), 1407 deletions(-) delete mode 100644 score/evaluation/EVALUATION_GUIDE.md delete mode 100644 score/evaluation/concurrency/EVALUATION_GUIDE.md delete mode 100644 score/evaluation/design/EVALUATION_GUIDE.md delete mode 100644 score/evaluation/exception_handling/EVALUATION_GUIDE.md delete mode 100644 score/evaluation/idiomatic/EVALUATION_GUIDE.md delete mode 100644 score/evaluation/memory/EVALUATION_GUIDE.md delete mode 100644 score/evaluation/modern_cpp/EVALUATION_GUIDE.md delete mode 100644 score/evaluation/optimization/EVALUATION_GUIDE.md delete mode 100644 score/evaluation/patterns/EVALUATION_GUIDE.md delete mode 100644 score/evaluation/safety/EVALUATION_GUIDE.md delete mode 100644 score/evaluation/security/EVALUATION_GUIDE.md delete mode 100644 score/evaluation/solid/EVALUATION_GUIDE.md delete mode 100644 score/evaluation/template_metaprogramming/EVALUATION_GUIDE.md delete mode 100644 score/evaluation/unit_test_quality/EVALUATION_GUIDE.md diff --git a/score/evaluation/BUILD b/score/evaluation/BUILD index 8bc02e7..cd69e00 100644 --- a/score/evaluation/BUILD +++ b/score/evaluation/BUILD @@ -1,26 +1,3 @@ -# ******************************************************************************* -# Copyright (c) 2025 Contributors to the Eclipse Foundation -# -# See the NOTICE file(s) distributed with this work for additional -# information regarding copyright ownership. -# -# This program and the accompanying materials are made available under the -# terms of the Apache License Version 2.0 which is available at -# https://www.apache.org/licenses/LICENSE-2.0 -# -# SPDX-License-Identifier: Apache-2.0 -# ******************************************************************************* -# -# BUILD file for score/evaluation -# -# Each cc_library target corresponds to one evaluation scenario file. -# The cc_binary target "evaluation_all" links every library and provides -# a single compilable artefact that the code-review tool can analyse. -# -# NOTE: These targets intentionally contain anti-patterns and bad practices. -# They are NOT suitable for production use and exist solely for -# evaluating the capability of automated code-review tools. -# ******************************************************************************* load("@rules_cc//cc:defs.bzl", "cc_binary") @@ -94,9 +71,6 @@ alias( actual = "//score/evaluation/unit_test_quality:unit_test_quality_test", ) -# --------------------------------------------------------------------------- -# Aggregate binary – compiles all evaluation scenarios together. -# --------------------------------------------------------------------------- cc_binary( name = "evaluation_all", srcs = ["evaluation_main.cpp"], diff --git a/score/evaluation/EVALUATION_GUIDE.md b/score/evaluation/EVALUATION_GUIDE.md deleted file mode 100644 index 99b5f71..0000000 --- a/score/evaluation/EVALUATION_GUIDE.md +++ /dev/null @@ -1,29 +0,0 @@ -# Evaluation Guide (Root Index) - -This root document is intentionally trimmed. -Use the per-directory guides for detailed test conditions and expected review comments. - -## Per-Directory Guides - -| Directory | Guide | -|---|---| -| `solid/` | [`solid/EVALUATION_GUIDE.md`](solid/EVALUATION_GUIDE.md) | -| `memory/` | [`memory/EVALUATION_GUIDE.md`](memory/EVALUATION_GUIDE.md) | -| `idiomatic/` | [`idiomatic/EVALUATION_GUIDE.md`](idiomatic/EVALUATION_GUIDE.md) | -| `modern_cpp/` | [`modern_cpp/EVALUATION_GUIDE.md`](modern_cpp/EVALUATION_GUIDE.md) | -| `patterns/` | [`patterns/EVALUATION_GUIDE.md`](patterns/EVALUATION_GUIDE.md) | -| `optimization/` | [`optimization/EVALUATION_GUIDE.md`](optimization/EVALUATION_GUIDE.md) | -| `security/` | [`security/EVALUATION_GUIDE.md`](security/EVALUATION_GUIDE.md) | -| `safety/` | [`safety/EVALUATION_GUIDE.md`](safety/EVALUATION_GUIDE.md) | -| `design/` | [`design/EVALUATION_GUIDE.md`](design/EVALUATION_GUIDE.md) | -| `concurrency/` | [`concurrency/EVALUATION_GUIDE.md`](concurrency/EVALUATION_GUIDE.md) | -| `exception_handling/` | [`exception_handling/EVALUATION_GUIDE.md`](exception_handling/EVALUATION_GUIDE.md) | -| `template_metaprogramming/` | [`template_metaprogramming/EVALUATION_GUIDE.md`](template_metaprogramming/EVALUATION_GUIDE.md) | -| `unit_test_quality/` | [`unit_test_quality/EVALUATION_GUIDE.md`](unit_test_quality/EVALUATION_GUIDE.md) | - -## How to Use - -1. Review code changes folder-by-folder. -2. Open the corresponding local `EVALUATION_GUIDE.md`. -3. Compare tool comments against expected findings in that local guide. -4. Record coverage per tag (`SV-*`, `MI-*`, `UT-*`, etc.) in your review notes. diff --git a/score/evaluation/concurrency/BUILD b/score/evaluation/concurrency/BUILD index 7b6cfd0..448dbbe 100644 --- a/score/evaluation/concurrency/BUILD +++ b/score/evaluation/concurrency/BUILD @@ -1,8 +1,3 @@ -# ******************************************************************************* -# Copyright (c) 2025 Contributors to the Eclipse Foundation -# -# SPDX-License-Identifier: Apache-2.0 -# ******************************************************************************* load("@rules_cc//cc:defs.bzl", "cc_library") diff --git a/score/evaluation/concurrency/EVALUATION_GUIDE.md b/score/evaluation/concurrency/EVALUATION_GUIDE.md deleted file mode 100644 index 0bec7d0..0000000 --- a/score/evaluation/concurrency/EVALUATION_GUIDE.md +++ /dev/null @@ -1,17 +0,0 @@ -# Concurrency Evaluation Guide - -## Scope -- **Source file:** `concurrency_issues.h` -- **Goal:** Verify tool catches race conditions, synchronization mistakes, and thread-lifetime bugs. - -## Test Conditions and Expected Review Comments -- **CI-01:** Data race on plain shared counter. -- **CI-02:** Manual lock/unlock without RAII (exception safety deadlock risk). -- **CI-03:** Broken double-checked init pattern / memory-ordering concerns. -- **CI-04:** Deadlock from inconsistent lock ordering. -- **CI-05:** Condition-variable wait not predicate-safe for spurious wakeups. -- **CI-06:** Detached thread captures stack reference (dangling reference risk). -- **CI-07:** Check-then-act race despite atomic variable. - -## Pass Criteria -- Tool should identify concrete interleavings/failure modes and propose robust synchronization patterns (`lock_guard`, `scoped_lock`, predicate waits, CAS loops). diff --git a/score/evaluation/concurrency/concurrency_issues.h b/score/evaluation/concurrency/concurrency_issues.h index 1a6d87b..8d4b14a 100644 --- a/score/evaluation/concurrency/concurrency_issues.h +++ b/score/evaluation/concurrency/concurrency_issues.h @@ -1,27 +1,4 @@ -// ******************************************************************************* -// Copyright (c) 2025 Contributors to the Eclipse Foundation -// -// See the NOTICE file(s) distributed with this work for additional -// information regarding copyright ownership. -// -// This program and the accompanying materials are made available under the -// terms of the Apache License Version 2.0 which is available at -// https://www.apache.org/parameters/LICENSE-2.0 -// -// SPDX-License-Identifier: Apache-2.0 -// ******************************************************************************* - -/// @file concurrency_issues.h -/// @brief Concurrency and thread-safety issues for code-review tool evaluation. -/// -/// Issues planted (see EVALUATION_GUIDE.md §9 for expected review comments): -/// [CI-01] Data race: shared counter incremented without synchronisation. -/// [CI-02] Mutex locked but not protected by RAII lock guard – leak on exception. -/// [CI-03] Double-checked locking without std::atomic – broken on modern hardware. -/// [CI-04] Deadlock: two mutexes acquired in inconsistent order across two functions. -/// [CI-05] Condition variable spurious wakeup not handled (while replaced by if). -/// [CI-06] Detached thread writing to stack variable of the spawning scope. -/// [CI-07] std::atomic used for a compound check-then-act – still a TOCTOU race. + #ifndef SCORE_EVALUATION_CONCURRENCY_ISSUES_H #define SCORE_EVALUATION_CONCURRENCY_ISSUES_H @@ -37,60 +14,46 @@ namespace score namespace evaluation { -// --------------------------------------------------------------------------- -// [CI-01] Data race on shared counter – no synchronisation. -// --------------------------------------------------------------------------- class UnsafeCounter { public: - void Increment() { ++count_; } // [CI-01] non-atomic, non-locked; data race + void Increment() { ++count_; } int Get() const { return count_; } private: - int count_{0}; // should be std::atomic or guarded by a mutex + int count_{0}; }; -// --------------------------------------------------------------------------- -// [CI-02] Manual mutex lock/unlock without RAII – leak on exception. -// --------------------------------------------------------------------------- class ManualLockUser { public: void Write(const std::string& data) { - mtx_.lock(); // [CI-02] if anything between lock and unlock throws, - ProcessData(data); // the mutex is never unlocked → deadlock. - mtx_.unlock(); // Use std::lock_guard lk(mtx_); + mtx_.lock(); + ProcessData(data); + mtx_.unlock(); } private: static void ProcessData(const std::string& /*data*/) { - // may throw } std::mutex mtx_; }; -// --------------------------------------------------------------------------- -// [CI-03] Double-checked locking without proper atomics / memory fences. -// --------------------------------------------------------------------------- class BrokenLazyInit { public: - // [CI-03] initialised_ is a plain bool – reads/writes are not atomic. - // A second thread may observe initialised_ == true before the - // constructor of resource_ has completed (memory reorder). - // Use std::atomic with acquire/release semantics or call_once. void EnsureInitialised() { - if (!initialised_) // first check, no lock + if (!initialised_) { std::lock_guard lk(mtx_); - if (!initialised_) // second check, under lock + if (!initialised_) { resource_ = 42; - initialised_ = true; // reorder: may be visible before resource_ = 42 + initialised_ = true; } } } @@ -98,30 +61,26 @@ class BrokenLazyInit int GetResource() const { return resource_; } private: - bool initialised_{false}; // [CI-03] should be std::atomic + bool initialised_{false}; int resource_{0}; std::mutex mtx_; }; -// --------------------------------------------------------------------------- -// [CI-04] Deadlock: mutexes acquired in opposite order in two different paths. -// --------------------------------------------------------------------------- class DeadlockProne { public: - // Thread A calls TransferAB, Thread B calls TransferBA simultaneously → deadlock. void TransferAB(int amount) { - std::lock_guard la(mtx_a_); // locks A first - std::lock_guard lb(mtx_b_); // then B + std::lock_guard la(mtx_a_); + std::lock_guard lb(mtx_b_); balance_a_ -= amount; balance_b_ += amount; } void TransferBA(int amount) { - std::lock_guard lb(mtx_b_); // locks B first [CI-04] - std::lock_guard la(mtx_a_); // then A – opposite order + std::lock_guard lb(mtx_b_); + std::lock_guard la(mtx_a_); balance_b_ -= amount; balance_a_ += amount; } @@ -133,19 +92,13 @@ class DeadlockProne int balance_b_{1000}; }; -// --------------------------------------------------------------------------- -// [CI-05] Condition variable spurious wakeup handled with 'if' instead of 'while'. -// --------------------------------------------------------------------------- class SpuriousWakeupBug { public: void Wait() { std::unique_lock lk(mtx_); - // [CI-05] Using if: spurious wakeup proceeds even when ready_ is false. - // Must use while (!ready_) or the predicate overload of wait(). if (!ready_) cv_.wait(lk); - // Proceed – but ready_ might still be false after a spurious wakeup. } void Signal() @@ -161,38 +114,25 @@ class SpuriousWakeupBug bool ready_{false}; }; -// --------------------------------------------------------------------------- -// [CI-06] Detached thread capturing reference to local stack variable. -// --------------------------------------------------------------------------- inline void StartDetachedThread() { std::string local_config = "config_data"; - // [CI-06] local_config is on the stack of StartDetachedThread(). - // After the function returns, the thread holds a dangling reference. std::thread t([&local_config]() { - // Accessing local_config after StartDetachedThread() returns → UB. (void)local_config; }); t.detach(); - // Function returns here; local_config destroyed; thread still running. } -// --------------------------------------------------------------------------- -// [CI-07] Compound check-then-act on std::atomic is still a TOCTOU race. -// --------------------------------------------------------------------------- class TocTouAtomic { public: - // [CI-07] Even though count_ is atomic, the if-check and decrement are - // two separate operations. Another thread can decrement between the - // check and the decrement, driving count_ below zero. void ConsumeIfAvailable() { - if (count_.load() > 0) // check + if (count_.load() > 0) { - --count_; // act – not atomic with the check + --count_; } } @@ -202,7 +142,7 @@ class TocTouAtomic std::atomic count_{0}; }; -} // namespace evaluation -} // namespace score +} +} #endif // SCORE_EVALUATION_CONCURRENCY_ISSUES_H diff --git a/score/evaluation/design/BUILD b/score/evaluation/design/BUILD index f06fa74..22b2eac 100644 --- a/score/evaluation/design/BUILD +++ b/score/evaluation/design/BUILD @@ -1,8 +1,3 @@ -# ******************************************************************************* -# Copyright (c) 2025 Contributors to the Eclipse Foundation -# -# SPDX-License-Identifier: Apache-2.0 -# ******************************************************************************* load("@rules_cc//cc:defs.bzl", "cc_library") diff --git a/score/evaluation/design/EVALUATION_GUIDE.md b/score/evaluation/design/EVALUATION_GUIDE.md deleted file mode 100644 index 35ba52e..0000000 --- a/score/evaluation/design/EVALUATION_GUIDE.md +++ /dev/null @@ -1,19 +0,0 @@ -# Software Design Faults Evaluation Guide - -## Scope -- **Source file:** `design_faults.h` -- **Goal:** Verify tool catches higher-level design smells and maintainability risks. - -## Test Conditions and Expected Review Comments -- **DF-01:** Anemic domain model; expected comment suggests moving behavior to entity. -- **DF-02:** Temporal coupling (`Init()` before `Process()`); expected comment suggests enforced initialization strategy. -- **DF-03:** Feature envy; expected comment suggests moving behavior to owning type. -- **DF-04:** Duplicated policy constant (shotgun surgery risk). -- **DF-05:** Refused bequest / contract-breaking inheritance. -- **DF-06:** Inappropriate intimacy via `friend`. -- **DF-07:** Long parameter list; expected comment suggests parameter object. -- **DF-08:** Magic numbers in business logic. -- **DF-09:** Boolean-trap API shape. - -## Pass Criteria -- Tool should provide architecture-level remediation ideas and identify LSP/contract implications where applicable. diff --git a/score/evaluation/design/design_faults.h b/score/evaluation/design/design_faults.h index 0011a2d..eb70159 100644 --- a/score/evaluation/design/design_faults.h +++ b/score/evaluation/design/design_faults.h @@ -1,30 +1,4 @@ -// ******************************************************************************* -// Copyright (c) 2025 Contributors to the Eclipse Foundation -// -// See the NOTICE file(s) distributed with this work for additional -// information regarding copyright ownership. -// -// This program and the accompanying materials are made available under the -// terms of the Apache License Version 2.0 which is available at -// https://www.apache.org/licenses/LICENSE-2.0 -// -// SPDX-License-Identifier: Apache-2.0 -// ******************************************************************************* - -/// @file design_faults.h -/// @brief Software-design-level faults for code-review tool evaluation. -/// -/// Issues planted (see EVALUATION_GUIDE.md §8 for expected review comments): -/// [DF-01] Anemic domain model – all business logic outside the class that owns the data. -/// [DF-02] Temporal coupling – callers must invoke Init() before Use() with no enforcement. -/// [DF-03] Feature envy – method operates almost entirely on another class's fields. -/// [DF-04] Shotgun surgery – a single concept (timeout) is duplicated across several classes. -/// [DF-05] Refused bequest – subclass ignores/breaks inherited public interface. -/// [DF-06] Inappropriate intimacy – two classes directly access each other's private via friend. -/// [DF-07] Long parameter list – function with 8 positional arguments. -/// [DF-08] Magic numbers scattered throughout without named constants. -/// [DF-09] Boolean trap – function with multiple bool parameters hiding intent. -/// [DF-10] Circular include dependency risk – explicit comment calling it out. + #ifndef SCORE_EVALUATION_DESIGN_FAULTS_H #define SCORE_EVALUATION_DESIGN_FAULTS_H @@ -38,10 +12,7 @@ namespace score namespace evaluation { -// --------------------------------------------------------------------------- -// [DF-01] Anemic domain model – Order holds data, OrderProcessor holds all logic. -// --------------------------------------------------------------------------- -struct Order // [DF-01] pure data bag; no behaviour; violates domain-model principle +struct Order { int id{0}; double total{0.0}; @@ -49,7 +20,7 @@ struct Order // [DF-01] pure data bag; no behaviour; violates domain-mo std::string customer_name; }; -class OrderProcessor // [DF-01] all logic here; tight coupling to Order internals +class OrderProcessor { public: void Apply10PercentDiscount(Order& o) { o.total *= 0.90; } @@ -57,9 +28,6 @@ class OrderProcessor // [DF-01] all logic here; tight coupling to Order interna bool IsFreeShippingEligible(const Order& o) { return o.total > 100.0; } }; -// --------------------------------------------------------------------------- -// [DF-02] Temporal coupling – Init() must be called before Process(). -// --------------------------------------------------------------------------- class PipelineStage { public: @@ -67,7 +35,6 @@ class PipelineStage void Init() { - // [DF-02] Nothing stops a caller from calling Process() without Init(). initialised_ = true; } @@ -76,7 +43,6 @@ class PipelineStage if (!initialised_) { throw std::runtime_error("PipelineStage: Init() not called"); - // Enforcement is runtime-only; should be enforced at construction time. } (void)data; } @@ -85,23 +51,19 @@ class PipelineStage bool initialised_; }; -// --------------------------------------------------------------------------- -// [DF-03] Feature envy – Renderer accesses Canvas fields directly. -// --------------------------------------------------------------------------- -struct Canvas // data holder +struct Canvas { int width{0}; int height{0}; std::vector pixels; }; -class Renderer // [DF-03] operates almost entirely on Canvas's internals +class Renderer { public: void Fill(Canvas& c, int colour) { c.pixels.assign(static_cast(c.width * c.height), colour); - // All work done with Canvas data; Fill() should be a method of Canvas. } void Resize(Canvas& c, int w, int h) @@ -109,37 +71,30 @@ class Renderer // [DF-03] operates almost entirely on Canvas's internals c.width = w; c.height = h; c.pixels.resize(static_cast(w * h), 0); - // Again: should be Canvas::Resize(). } }; -// --------------------------------------------------------------------------- -// [DF-04] Shotgun surgery – timeout value duplicated across unrelated classes. -// --------------------------------------------------------------------------- class NetworkClient { - static constexpr int kTimeoutMs = 5000; // [DF-04] duplicated magic value + static constexpr int kTimeoutMs = 5000; public: void Connect() { /* uses kTimeoutMs */ } }; class FileWatcher { - static constexpr int kTimeoutMs = 5000; // [DF-04] same constant, no shared source + static constexpr int kTimeoutMs = 5000; public: void Watch() { /* uses kTimeoutMs */ } }; class HealthCheck { - static constexpr int kTimeoutMs = 5000; // [DF-04] three places to change if requirement changes + static constexpr int kTimeoutMs = 5000; public: void Ping() { /* uses kTimeoutMs */ } }; -// --------------------------------------------------------------------------- -// [DF-05] Refused bequest – ReadOnlyList claims to be a List but rejects mutations. -// --------------------------------------------------------------------------- class List { public: @@ -150,12 +105,12 @@ class List virtual int Size() const = 0; }; -class ReadOnlyList : public List // [DF-05] refuses inherited mutation contract +class ReadOnlyList : public List { public: void Add(int /*value*/) override { - throw std::runtime_error("ReadOnlyList: Add not supported"); // breaks LSP too + throw std::runtime_error("ReadOnlyList: Add not supported"); } void Remove(int /*value*/) override { @@ -168,20 +123,17 @@ class ReadOnlyList : public List // [DF-05] refuses inherited mutation con std::vector data_; }; -// --------------------------------------------------------------------------- -// [DF-06] Inappropriate intimacy via friend class. -// --------------------------------------------------------------------------- class AccountLedger; class AuditEngine { public: - void Audit(AccountLedger& ledger); // accesses private members via friend + void Audit(AccountLedger& ledger); }; class AccountLedger { - friend class AuditEngine; // [DF-06] tight coupling; expose only what's needed via accessors + friend class AuditEngine; private: double balance_{0.0}; std::vector transactions_; @@ -189,15 +141,11 @@ class AccountLedger inline void AuditEngine::Audit(AccountLedger& ledger) { - // Direct access to private members breaks encapsulation. if (ledger.balance_ < 0) { /* flag */ } for (auto t : ledger.transactions_) { (void)t; } } -// --------------------------------------------------------------------------- -// [DF-07] Long parameter list – 8 positional arguments. -// --------------------------------------------------------------------------- -inline void ConfigureConnection( // [DF-07]: wrap in a ConnectionConfig struct +inline void ConfigureConnection( const std::string& host, int port, int timeout_ms, @@ -211,30 +159,24 @@ inline void ConfigureConnection( // [DF-07]: wrap in a ConnectionConfi (void)use_tls; (void)verify_cert; (void)username; (void)password; } -// --------------------------------------------------------------------------- -// [DF-08] Magic numbers throughout logic without named constants. -// --------------------------------------------------------------------------- inline double CalculateScore(int raw) { - if (raw > 255) return 0.0; // [DF-08]: what is 255? - double norm = raw / 128.0; // [DF-08]: what is 128? - if (norm > 1.75) return 100.0; // [DF-08]: what is 1.75? - return norm * 57.14; // [DF-08]: what is 57.14? + if (raw > 255) return 0.0; + double norm = raw / 128.0; + if (norm > 1.75) return 100.0; + return norm * 57.14; } -// --------------------------------------------------------------------------- -// [DF-09] Boolean trap – multiple bool parameters hide call-site intent. -// --------------------------------------------------------------------------- -inline void ExportData( // [DF-09]: use enum class or named-parameter idiom +inline void ExportData( const std::string& filename, - bool compress, // caller: ExportData("out", true, false, true) – unreadable + bool compress, bool encrypt, bool overwrite) { (void)filename; (void)compress; (void)encrypt; (void)overwrite; } -} // namespace evaluation -} // namespace score +} +} #endif // SCORE_EVALUATION_DESIGN_FAULTS_H diff --git a/score/evaluation/evaluation_main.cpp b/score/evaluation/evaluation_main.cpp index 4c5db6a..c83f564 100644 --- a/score/evaluation/evaluation_main.cpp +++ b/score/evaluation/evaluation_main.cpp @@ -1,19 +1,4 @@ -// ******************************************************************************* -// Copyright (c) 2025 Contributors to the Eclipse Foundation -// -// See the NOTICE file(s) distributed with this work for additional -// information regarding copyright ownership. -// -// This program and the accompanying materials are made available under the -// terms of the Apache License Version 2.0 which is available at -// https://www.apache.org/licenses/LICENSE-2.0 -// -// SPDX-License-Identifier: Apache-2.0 -// ******************************************************************************* - -/// @file evaluation_main.cpp -/// @brief Entry point that exercises all planted issues so that every -/// cc_binary target compiles and the review tool can analyse them. + #include "score/evaluation/concurrency/concurrency_issues.h" #include "score/evaluation/design/design_faults.h" @@ -30,7 +15,6 @@ int main() { - // --- SOLID violations --- score::evaluation::LogManager lm; lm.LoadConfig("cfg.json"); lm.RouteMessage("hello"); @@ -42,7 +26,6 @@ int main() score::evaluation::HighLevelProcessor hlp; hlp.Process("data"); - // --- Memory issues --- score::evaluation::RawOwner ro(10); ro.At(0) = 42; @@ -52,50 +35,43 @@ int main() score::evaluation::FillBuffer(2, 'X'); score::evaluation::SelfAssignBroken sab(4); - sab = sab; // triggers self-assignment bug + sab = sab; - // --- Non-idiomatic C++ --- score::evaluation::ToDouble(5); score::evaluation::SumVector({1, 2, 3}); score::evaluation::HasPrefix("hello world", "hello"); score::evaluation::PrintLines({"line1", "line2"}); - // --- Modern C++ syntax --- score::evaluation::StringList sl{"alpha", "beta", "gamma_long_string"}; score::evaluation::CountNonEmpty(sl); score::evaluation::FilterLong(sl); score::evaluation::MakeAdder(10)(5); - score::evaluation::InitialiseSubsystem(0); // return value ignored + score::evaluation::InitialiseSubsystem(0); - // --- Design patterns --- score::evaluation::BrokenSingleton::GetInstance()->DoWork(); score::evaluation::WidgetFactory wf; score::evaluation::Widget* w = wf.Create("button"); - delete w; // caller must remember to delete + delete w; score::evaluation::BubbleSort bs; score::evaluation::Sorter sorter(&bs); std::vector nums{3, 1, 4, 1, 5}; sorter.Sort(nums); - // --- Code optimisation --- score::evaluation::SumLargeVector({1, 2, 3, 4, 5}); score::evaluation::IsError("ERROR"); score::evaluation::JoinLines({"a", "b", "c"}); score::evaluation::SortAndSlice({5, 3, 1, 4, 2}); - // --- Safe C++ --- std::vector v{1, 2, 3}; - score::evaluation::IndexInRange(-1, v); // signed/unsigned comparison + score::evaluation::IndexInRange(-1, v); score::evaluation::MultiplyUnchecked(100000, 100000); score::evaluation::TruncateToByte(300); score::evaluation::CountFlags(true, false, true); - // --- Security issues --- score::evaluation::EvaluateSecuritySamples(); - // --- Design faults --- score::evaluation::Order order{1, 150.0, false, "Alice"}; score::evaluation::OrderProcessor op; op.Apply10PercentDiscount(order); @@ -103,19 +79,16 @@ int main() score::evaluation::CalculateScore(200); score::evaluation::ExportData("out.bin", true, false, true); - // --- Concurrency issues --- score::evaluation::UnsafeCounter uc; uc.Increment(); score::evaluation::SpuriousWakeupBug swb; swb.Signal(); - // --- Exception / error handling --- score::evaluation::CatchByValue(); score::evaluation::LoadFile("nonexistent.txt"); - score::evaluation::InitDevice(0); // return value intentionally ignored + score::evaluation::InitDevice(0); - // --- Template metaprogramming issues --- score::evaluation::EvaluateTemplateMetaprogrammingSamples(); return 0; diff --git a/score/evaluation/exception_handling/BUILD b/score/evaluation/exception_handling/BUILD index c7b1f48..a66e353 100644 --- a/score/evaluation/exception_handling/BUILD +++ b/score/evaluation/exception_handling/BUILD @@ -1,8 +1,3 @@ -# ******************************************************************************* -# Copyright (c) 2025 Contributors to the Eclipse Foundation -# -# SPDX-License-Identifier: Apache-2.0 -# ******************************************************************************* load("@rules_cc//cc:defs.bzl", "cc_library") diff --git a/score/evaluation/exception_handling/EVALUATION_GUIDE.md b/score/evaluation/exception_handling/EVALUATION_GUIDE.md deleted file mode 100644 index 12f05f7..0000000 --- a/score/evaluation/exception_handling/EVALUATION_GUIDE.md +++ /dev/null @@ -1,17 +0,0 @@ -# Exception & Error Handling Evaluation Guide - -## Scope -- **Source file:** `exception_error_handling.h` -- **Goal:** Verify tool catches incorrect exception patterns and weak error contracts. - -## Test Conditions and Expected Review Comments -- **EH-02:** Silent catch-all that suppresses diagnostics. -- **EH-03:** Throwing raw/non-standard exception types. -- **EH-04:** Using exceptions for normal control flow. -- **EH-05:** `noexcept` function with throwing operations. -- **EH-06:** Incorrect rethrow pattern losing original exception context. -- **EH-07:** Plain integer error codes easily ignored. -- **EH-08:** Constructor failure swallowed, object left invalid. - -## Pass Criteria -- Tool should recommend explicit, consistent error contracts (`std::exception`-derived types, `[[nodiscard]]`, structured error handling). diff --git a/score/evaluation/exception_handling/exception_error_handling.h b/score/evaluation/exception_handling/exception_error_handling.h index ba5476e..31ce5b8 100644 --- a/score/evaluation/exception_handling/exception_error_handling.h +++ b/score/evaluation/exception_handling/exception_error_handling.h @@ -1,27 +1,4 @@ -// ******************************************************************************* -// Copyright (c) 2025 Contributors to the Eclipse Foundation -// -// See the NOTICE file(s) distributed with this work for additional -// information regarding copyright ownership. -// -// This program and the accompanying materials are made available under the -// terms of the Apache License Version 2.0 which is available at -// https://www.apache.org/licenses/LICENSE-2.0 -// -// SPDX-License-Identifier: Apache-2.0 -// ******************************************************************************* - -/// @file exception_error_handling.h -/// @brief Exception and error-handling issues for code-review tool evaluation. -/// -/// Issues planted (see EVALUATION_GUIDE.md §10 for expected review comments): -/// [EH-02] Swallowing exceptions silently in a catch-all block. -/// [EH-03] Throwing std::string / int instead of a proper exception type. -/// [EH-04] Using exception for normal control flow (not an error condition). -/// [EH-05] noexcept function that calls a function which can throw. -/// [EH-06] Re-throwing with 'throw ex' (slices the exception) vs 'throw'. -/// [EH-07] Error code returned as int – easy for caller to ignore silently. -/// [EH-08] Constructor that silently swallows failure – leaves object half-initialised. + #ifndef SCORE_EVALUATION_EXCEPTION_ERROR_HANDLING_H #define SCORE_EVALUATION_EXCEPTION_ERROR_HANDLING_H @@ -36,9 +13,6 @@ namespace score namespace evaluation { -// --------------------------------------------------------------------------- -// Catching by reference (compiler-detectable catch-by-value condition removed). -// --------------------------------------------------------------------------- inline void CatchByValue() { try @@ -51,9 +25,6 @@ inline void CatchByValue() } } -// --------------------------------------------------------------------------- -// [EH-02] Silent catch-all swallows exceptions without logging or re-throw. -// --------------------------------------------------------------------------- inline bool LoadFile(const std::string& path) { try @@ -62,56 +33,41 @@ inline bool LoadFile(const std::string& path) if (!f.is_open()) throw std::runtime_error("cannot open"); return true; } - catch (...) // [EH-02] all exceptions silently discarded; caller gets false - { // with no diagnosis. At minimum log, or std::throw_with_nested. + catch (...) + { return false; } } -// --------------------------------------------------------------------------- -// [EH-03] Throwing a raw string / int instead of a proper exception type. -// --------------------------------------------------------------------------- inline void ParseConfig(const std::string& cfg) { if (cfg.empty()) { - throw "config must not be empty"; // [EH-03] throwing const char*; cannot catch via - // std::exception; use std::invalid_argument + throw "config must not be empty"; } if (cfg.size() > 4096) { - throw 42; // [EH-03] throwing int; completely non-standard + throw 42; } } -// --------------------------------------------------------------------------- -// [EH-04] Exception used for normal (non-error) control flow. -// --------------------------------------------------------------------------- inline int FindIndex(const std::vector& v, int target) { - // [EH-04] Throws when target is simply not found – a common, non-exceptional case. - // Should return std::optional or -1 / a sentinel. for (std::size_t i = 0; i < v.size(); ++i) { if (v[i] == target) return static_cast(i); } - throw std::runtime_error("element not found"); // not an error; normal outcome + throw std::runtime_error("element not found"); } -// --------------------------------------------------------------------------- -// [EH-05] noexcept function calling a potentially throwing function. -// --------------------------------------------------------------------------- -inline std::string ReadLine(const std::string& path) noexcept // [EH-05] promises noexcept +inline std::string ReadLine(const std::string& path) noexcept { std::ifstream f(path); std::string line; - std::getline(f, line); // can throw; if it does, std::terminate is called + std::getline(f, line); return line; } -// --------------------------------------------------------------------------- -// [EH-06] Re-throwing by value slices the exception. -// --------------------------------------------------------------------------- inline void Process() { try @@ -120,28 +76,18 @@ inline void Process() } catch (const std::exception& ex) { - // [EH-06] Copies and slices the exception; dynamic type lost. - // Use bare 'throw;' to re-throw the original object in-place. std::runtime_error copy = std::runtime_error(ex.what()); throw copy; } } -// --------------------------------------------------------------------------- -// [EH-07] Error code returned as plain int – easily ignored by caller. -// --------------------------------------------------------------------------- -// [EH-07] Returning int error codes is fragile; use [[nodiscard]] + error enum, -// std::error_code, or expected. No nodiscard attribute here. int InitDevice(int device_id); inline int InitDevice(int /*device_id*/) { - return -1; // error; caller may write: InitDevice(3); with no warning + return -1; } -// --------------------------------------------------------------------------- -// [EH-08] Constructor silently swallowing failure – half-initialised object. -// --------------------------------------------------------------------------- class SilentlyFailingResource { public: @@ -150,18 +96,14 @@ class SilentlyFailingResource file_.open(path); if (!file_.is_open()) { - // [EH-08] Should throw std::runtime_error or propagate via factory function. - // Instead, object is constructed in an invalid state; - // callers have no way to know without checking IsValid(). } - // No throw → object silently broken } - bool IsValid() const { return file_.is_open(); } // callers must remember to check + bool IsValid() const { return file_.is_open(); } std::string ReadLine() { - if (!IsValid()) return {}; // silent failure + if (!IsValid()) return {}; std::string line; std::getline(file_, line); return line; @@ -171,7 +113,7 @@ class SilentlyFailingResource std::ifstream file_; }; -} // namespace evaluation -} // namespace score +} +} #endif // SCORE_EVALUATION_EXCEPTION_ERROR_HANDLING_H diff --git a/score/evaluation/idiomatic/BUILD b/score/evaluation/idiomatic/BUILD index b88dc18..5187b1b 100644 --- a/score/evaluation/idiomatic/BUILD +++ b/score/evaluation/idiomatic/BUILD @@ -1,8 +1,3 @@ -# ******************************************************************************* -# Copyright (c) 2025 Contributors to the Eclipse Foundation -# -# SPDX-License-Identifier: Apache-2.0 -# ******************************************************************************* load("@rules_cc//cc:defs.bzl", "cc_library") diff --git a/score/evaluation/idiomatic/EVALUATION_GUIDE.md b/score/evaluation/idiomatic/EVALUATION_GUIDE.md deleted file mode 100644 index 4d4e8c0..0000000 --- a/score/evaluation/idiomatic/EVALUATION_GUIDE.md +++ /dev/null @@ -1,20 +0,0 @@ -# Non-Idiomatic C++ Evaluation Guide - -## Scope -- **Source file:** `non_idiomatic_cpp.h` -- **Goal:** Verify tool catches non-idiomatic legacy patterns and recommends modern C++ style. - -## Test Conditions and Expected Review Comments -- **NI-01:** C-style casts; expected comment suggests named casts. -- **NI-02:** `NULL` usage; expected comment suggests `nullptr`. -- **NI-03:** `#define` constants; expected comment suggests `constexpr`. -- **NI-04:** C-style arrays; expected comment suggests `std::array`. -- **NI-05:** `sprintf`; expected comment flags safety and suggests safer formatting. -- **NI-06:** Manual loops for accumulation; expected comment suggests STL algorithms. -- **NI-07:** Mutable global state in header; expected comment flags coupling/ODR/thread-safety concerns. -- **NI-08:** `std::string` pass-by-value; expected comment suggests `const&` or `std::string_view`. -- **NI-09:** `std::endl` flush in loop; expected comment suggests `"\n"`. -- **NI-10:** Out-parameter parse API; expected comment suggests value-return (`std::optional`). - -## Pass Criteria -- Tool should emit actionable modernization guidance beyond formatting-only remarks. diff --git a/score/evaluation/idiomatic/non_idiomatic_cpp.h b/score/evaluation/idiomatic/non_idiomatic_cpp.h index 190787a..d393fd6 100644 --- a/score/evaluation/idiomatic/non_idiomatic_cpp.h +++ b/score/evaluation/idiomatic/non_idiomatic_cpp.h @@ -1,30 +1,4 @@ -// ******************************************************************************* -// Copyright (c) 2025 Contributors to the Eclipse Foundation -// -// See the NOTICE file(s) distributed with this work for additional -// information regarding copyright ownership. -// -// This program and the accompanying materials are made available under the -// terms of the Apache License Version 2.0 which is available at -// https://www.apache.org/licenses/LICENSE-2.0 -// -// SPDX-License-Identifier: Apache-2.0 -// ******************************************************************************* - -/// @file non_idiomatic_cpp.h -/// @brief Non-idiomatic C++ patterns for code-review tool evaluation. -/// -/// Issues planted (see EVALUATION_GUIDE.md §3 for expected review comments): -/// [NI-01] C-style cast instead of static_cast / reinterpret_cast. -/// [NI-02] NULL macro instead of nullptr. -/// [NI-03] #define constant instead of constexpr. -/// [NI-04] C-style array instead of std::array. -/// [NI-05] printf / sprintf instead of std::to_string or streams. -/// [NI-06] Manual loop instead of std algorithm (std::accumulate, etc.). -/// [NI-07] Mutable global state / non-const global variable. -/// [NI-08] Passing std::string by value where const-ref or string_view suffices. -/// [NI-09] Using std::endl (flushes) instead of '\n'. -/// [NI-10] Out-parameters (raw pointer out-param) instead of returning value/pair/struct. + #ifndef SCORE_EVALUATION_NON_IDIOMATIC_CPP_H #define SCORE_EVALUATION_NON_IDIOMATIC_CPP_H @@ -40,108 +14,78 @@ namespace score namespace evaluation { -// --------------------------------------------------------------------------- -// [NI-03] #define constant – should use constexpr. -// --------------------------------------------------------------------------- #define MAX_ITEMS 64 // [NI-03]: prefer constexpr int kMaxItems = 64; #define PI_VALUE 3.14159265 // [NI-03]: prefer constexpr double kPi = 3.14159265; -// --------------------------------------------------------------------------- -// [NI-07] Mutable non-const global variable. -// --------------------------------------------------------------------------- -int g_request_count = 0; // [NI-07]: global mutable state; prefer encapsulation +int g_request_count = 0; -// --------------------------------------------------------------------------- -// [NI-01] C-style casts. -// --------------------------------------------------------------------------- inline double ToDouble(int value) { - return (double)value; // [NI-01]: use static_cast(value) + return (double)value; } inline void* GetRawPtr(int* p) { - return (void*)p; // [NI-01]: use static_cast or reinterpret_cast with comment + return (void*)p; } -// --------------------------------------------------------------------------- -// [NI-02] NULL instead of nullptr. -// --------------------------------------------------------------------------- inline bool IsNull(int* ptr) { - return ptr == NULL; // [NI-02]: use nullptr + return ptr == NULL; } void SetPointer(int** pp) { - *pp = NULL; // [NI-02]: use nullptr + *pp = NULL; } -// --------------------------------------------------------------------------- -// [NI-04] C-style array instead of std::array. -// --------------------------------------------------------------------------- struct LegacyBuffer { - int data[MAX_ITEMS]; // [NI-04]: prefer std::array + int data[MAX_ITEMS]; int size; void Clear() { - for (int i = 0; i < MAX_ITEMS; ++i) // [NI-04] + [NI-06] + for (int i = 0; i < MAX_ITEMS; ++i) { data[i] = 0; } } }; -// --------------------------------------------------------------------------- -// [NI-05] printf / sprintf instead of C++ streams or std::to_string. -// --------------------------------------------------------------------------- inline std::string FormatMessage(int code, const std::string& text) { char buf[256]; - sprintf(buf, "Code=%d Msg=%s", code, text.c_str()); // [NI-05]: unsafe, use snprintf or streams + sprintf(buf, "Code=%d Msg=%s", code, text.c_str()); return std::string(buf); } -// --------------------------------------------------------------------------- -// [NI-06] Manual accumulation loop – should use std::accumulate. -// --------------------------------------------------------------------------- inline int SumVector(const std::vector& vec) { int total = 0; - for (int i = 0; i < (int)vec.size(); ++i) // [NI-06] + [NI-01] C-style cast + for (int i = 0; i < (int)vec.size(); ++i) { total += vec[i]; } return total; } -// --------------------------------------------------------------------------- -// [NI-08] std::string passed by value – should be const std::string& or std::string_view. -// --------------------------------------------------------------------------- -inline bool HasPrefix(std::string text, std::string prefix) // [NI-08]: unnecessary copy +inline bool HasPrefix(std::string text, std::string prefix) { return text.substr(0, prefix.size()) == prefix; } -// --------------------------------------------------------------------------- -// [NI-09] std::endl instead of '\n' – unnecessary flush. -// --------------------------------------------------------------------------- inline void PrintLines(const std::vector& lines) { for (const auto& line : lines) { - std::cout << line << std::endl; // [NI-09]: prefer '\n'; endl flushes the buffer + std::cout << line << std::endl; } } -// --------------------------------------------------------------------------- -// [NI-10] Out-parameter instead of returning a value. -// --------------------------------------------------------------------------- -inline bool ParseInt(const std::string& str, int* result) // [NI-10]: out-param anti-pattern +inline bool ParseInt(const std::string& str, int* result) { - if (str.empty() || result == NULL) // [NI-02] reuse + if (str.empty() || result == NULL) { return false; } @@ -149,7 +93,7 @@ inline bool ParseInt(const std::string& str, int* result) // [NI-10]: out-para return true; } -} // namespace evaluation -} // namespace score +} +} #endif // SCORE_EVALUATION_NON_IDIOMATIC_CPP_H diff --git a/score/evaluation/memory/BUILD b/score/evaluation/memory/BUILD index 9723f7d..3848a3b 100644 --- a/score/evaluation/memory/BUILD +++ b/score/evaluation/memory/BUILD @@ -1,8 +1,3 @@ -# ******************************************************************************* -# Copyright (c) 2025 Contributors to the Eclipse Foundation -# -# SPDX-License-Identifier: Apache-2.0 -# ******************************************************************************* load("@rules_cc//cc:defs.bzl", "cc_library") diff --git a/score/evaluation/memory/EVALUATION_GUIDE.md b/score/evaluation/memory/EVALUATION_GUIDE.md deleted file mode 100644 index 9f55350..0000000 --- a/score/evaluation/memory/EVALUATION_GUIDE.md +++ /dev/null @@ -1,16 +0,0 @@ -# Memory Evaluation Guide - -## Scope -- **Source file:** `memory_issues.h` -- **Goal:** Verify tool catches lifetime/ownership and UB-prone memory patterns. - -## Test Conditions and Expected Review Comments -- **MI-01:** Owning raw pointer in `RawOwner`; expected comment suggests RAII (`std::vector`/`std::unique_ptr`). -- **MI-02:** Missing virtual destructor in polymorphic base; expected comment flags UB on delete-through-base. -- **MI-03:** Rule-of-Five break causes double-free risk; expected comment asks for Rule-of-Zero/Rule-of-Five fix. -- **MI-04:** Constructor exception leak path; expected comment recommends RAII member ownership. -- **MI-05:** Vector pointer invalidation then dereference; expected comment flags dangling pointer/UAF. -- **MI-08:** Copy-assignment self-assignment bug; expected comment suggests guard or copy-swap idiom. - -## Pass Criteria -- Tool should report high-severity memory/lifetime defects and explain runtime impact (UB, leaks, double-free). diff --git a/score/evaluation/memory/memory_issues.h b/score/evaluation/memory/memory_issues.h index 1b3cd98..a28d3fb 100644 --- a/score/evaluation/memory/memory_issues.h +++ b/score/evaluation/memory/memory_issues.h @@ -1,26 +1,4 @@ -// ******************************************************************************* -// Copyright (c) 2025 Contributors to the Eclipse Foundation -// -// See the NOTICE file(s) distributed with this work for additional -// information regarding copyright ownership. -// -// This program and the accompanying materials are made available under the -// terms of the Apache License Version 2.0 which is available at -// https://www.apache.org/licenses/LICENSE-2.0 -// -// SPDX-License-Identifier: Apache-2.0 -// ******************************************************************************* - -/// @file memory_issues.h -/// @brief Intentional memory-management issues for code-review tool evaluation. -/// -/// Issues planted (see EVALUATION_GUIDE.md §2 for expected review comments): -/// [MI-01] Raw owning pointer – manual new/delete, no RAII. -/// [MI-02] Missing virtual destructor on polymorphic base → UB on delete. -/// [MI-03] Double-free risk in destructor after copy construction. -/// [MI-04] Memory leak on exception path inside constructor. -/// [MI-05] Use-after-free: pointer stored after vector reallocation. -/// [MI-08] Self-assignment not handled in copy-assignment operator. + #ifndef SCORE_EVALUATION_MEMORY_ISSUES_H #define SCORE_EVALUATION_MEMORY_ISSUES_H @@ -35,9 +13,6 @@ namespace score namespace evaluation { -// --------------------------------------------------------------------------- -// [MI-01] Raw owning pointer – should use std::unique_ptr. -// --------------------------------------------------------------------------- class RawOwner { public: @@ -45,24 +20,20 @@ class RawOwner ~RawOwner() { - delete[] data_; // will leak if constructor throws after allocation + delete[] data_; } - int& At(int index) { return data_[index]; } // no bounds check + int& At(int index) { return data_[index]; } private: - int* data_; // owning raw pointer + int* data_; int size_; }; -// --------------------------------------------------------------------------- -// [MI-02] Missing virtual destructor – UB when Base* points to Derived. -// --------------------------------------------------------------------------- class ResourceBase { public: - // No virtual destructor: deleting via base pointer causes undefined behaviour. - ~ResourceBase() {} // [MI-02]: should be virtual ~ResourceBase() + ~ResourceBase() {} virtual void Process() = 0; }; @@ -71,7 +42,7 @@ class ResourceDerived : public ResourceBase { public: explicit ResourceDerived(int n) : buf_(new char[n]) {} - ~ResourceDerived() { delete[] buf_; } // never called via ResourceBase* + ~ResourceDerived() { delete[] buf_; } void Process() override {} @@ -79,34 +50,24 @@ class ResourceDerived : public ResourceBase char* buf_; }; -// --------------------------------------------------------------------------- -// [MI-03] Double-free via implicit copy – no copy constructor or Rule-of-Five. -// --------------------------------------------------------------------------- class DoubleFreeProne { public: explicit DoubleFreeProne(std::size_t n) : ptr_(new int[n]) {} ~DoubleFreeProne() { delete[] ptr_; } - // No user-defined copy constructor: compiler-generated does a shallow copy. - // Copying two instances then destructing both → double free of ptr_. - private: - int* ptr_; // raw owning pointer without Rule-of-Five + int* ptr_; }; -// --------------------------------------------------------------------------- -// [MI-04] Memory leak on exception path. -// --------------------------------------------------------------------------- class LeakyConstructor { public: LeakyConstructor() { buffer_ = new char[1024]; - another_ = new char[512]; // if this throws, buffer_ leaks - // Should use unique_ptr or initialise via constructor-body members. - MightThrow(); // if this throws, both allocations leak + another_ = new char[512]; + MightThrow(); } ~LeakyConstructor() @@ -122,9 +83,6 @@ class LeakyConstructor char* another_{nullptr}; }; -// --------------------------------------------------------------------------- -// [MI-05] Use-after-free / dangling pointer after vector resize. -// --------------------------------------------------------------------------- class VectorPointerInvalidation { public: @@ -138,12 +96,10 @@ class VectorPointerInvalidation items_.reserve(4); items_.push_back(10); - const int* ptr = &items_[0]; // pointer into vector storage + const int* ptr = &items_[0]; - // Causes reallocation → ptr is now dangling for (int i = 0; i < 100; ++i) { items_.push_back(i); } - // [MI-05] Reading through invalidated pointer – undefined behaviour. int val = *ptr; (void)val; } @@ -152,9 +108,6 @@ class VectorPointerInvalidation std::vector items_; }; -// --------------------------------------------------------------------------- -// Safe buffer write helper (compiler-detectable overflow condition removed). -// --------------------------------------------------------------------------- constexpr int kBufSize = 16; void FillBuffer(int user_index, char value) @@ -168,18 +121,12 @@ void FillBuffer(int user_index, char value) (void)buf[user_index]; } -// --------------------------------------------------------------------------- -// Safe value return helper (compiler-detectable dangling-reference condition removed). -// --------------------------------------------------------------------------- std::string GetLabel() { std::string local = "transient_label"; return local; } -// --------------------------------------------------------------------------- -// [MI-08] Self-assignment not handled in copy-assignment operator. -// --------------------------------------------------------------------------- class SelfAssignBroken { public: @@ -195,13 +142,12 @@ class SelfAssignBroken std::memcpy(data_, other.data_, size_ * sizeof(int)); } - // [MI-08] Self-assignment deletes data_ before copying from it. SelfAssignBroken& operator=(const SelfAssignBroken& other) { - delete[] data_; // danger: if other == *this, data_ gone + delete[] data_; size_ = other.size_; data_ = new int[size_]; - std::memcpy(data_, other.data_, size_ * sizeof(int)); // reads freed memory on self-assign + std::memcpy(data_, other.data_, size_ * sizeof(int)); return *this; } @@ -212,7 +158,7 @@ class SelfAssignBroken int* data_; }; -} // namespace evaluation -} // namespace score +} +} #endif // SCORE_EVALUATION_MEMORY_ISSUES_H diff --git a/score/evaluation/modern_cpp/BUILD b/score/evaluation/modern_cpp/BUILD index 557c3cd..e237f87 100644 --- a/score/evaluation/modern_cpp/BUILD +++ b/score/evaluation/modern_cpp/BUILD @@ -1,8 +1,3 @@ -# ******************************************************************************* -# Copyright (c) 2025 Contributors to the Eclipse Foundation -# -# SPDX-License-Identifier: Apache-2.0 -# ******************************************************************************* load("@rules_cc//cc:defs.bzl", "cc_library") diff --git a/score/evaluation/modern_cpp/EVALUATION_GUIDE.md b/score/evaluation/modern_cpp/EVALUATION_GUIDE.md deleted file mode 100644 index 01beef3..0000000 --- a/score/evaluation/modern_cpp/EVALUATION_GUIDE.md +++ /dev/null @@ -1,20 +0,0 @@ -# Modern C++ Evaluation Guide - -## Scope -- **Source file:** `modern_cpp_syntax.h` -- **Goal:** Verify tool identifies missed C++11/14/17 language/library opportunities. - -## Test Conditions and Expected Review Comments -- **MC-01:** Raw iterator loop; expected comment suggests range-for. -- **MC-02:** Legacy pointer idiom awareness; expected comment should discourage removed/obsolete ownership styles. -- **MC-03:** Verbose functor; expected comment suggests lambda. -- **MC-04:** Manual tagged union; expected comment suggests `std::variant` + `std::visit`. -- **MC-05:** Move operations not `noexcept`; expected comment explains container-move optimization impact. -- **MC-06:** `typedef` legacy style; expected comment suggests `using` alias. -- **MC-07:** Pair-based sentinel return; expected comment suggests `std::optional` / clearer API. -- **MC-08:** `map::operator[]` unintended insertion; expected comment suggests `find()`/`at()` depending on intent. -- **MC-09:** `std::bind` for simple closure; expected comment suggests lambda. -- **MC-10:** Missing `[[nodiscard]]` on status-returning API. - -## Pass Criteria -- Tool should connect suggestions to C++17 semantics and performance/safety implications. diff --git a/score/evaluation/modern_cpp/modern_cpp_syntax.h b/score/evaluation/modern_cpp/modern_cpp_syntax.h index 71744f0..26afe5a 100644 --- a/score/evaluation/modern_cpp/modern_cpp_syntax.h +++ b/score/evaluation/modern_cpp/modern_cpp_syntax.h @@ -1,30 +1,4 @@ -// ******************************************************************************* -// Copyright (c) 2025 Contributors to the Eclipse Foundation -// -// See the NOTICE file(s) distributed with this work for additional -// information regarding copyright ownership. -// -// This program and the accompanying materials are made available under the -// terms of the Apache License Version 2.0 which is available at -// https://www.apache.org/licenses/LICENSE-2.0 -// -// SPDX-License-Identifier: Apache-2.0 -// ******************************************************************************* - -/// @file modern_cpp_syntax.h -/// @brief Pre-C++11/14 patterns that should be replaced with C++11/14/17 idioms. -/// -/// Issues planted (see EVALUATION_GUIDE.md §4 for expected review comments): -/// [MC-01] Raw loop with iterator instead of range-for. -/// [MC-02] std::auto_ptr instead of std::unique_ptr (removed in C++17). -/// [MC-03] Explicit functor/struct for predicate instead of lambda. -/// [MC-04] Manual type-tag enum instead of std::variant (C++17). -/// [MC-05] Missing noexcept on move constructor/assignment. -/// [MC-06] Typedef instead of using alias. -/// [MC-07] Manual pair return instead of structured bindings (C++17). -/// [MC-08] Unchecked std::map::operator[] (inserts default) instead of find/at. -/// [MC-09] std::bind instead of lambda capture. -/// [MC-10] Missing [[nodiscard]] on error-signalling return value. + #ifndef SCORE_EVALUATION_MODERN_CPP_SYNTAX_H #define SCORE_EVALUATION_MODERN_CPP_SYNTAX_H @@ -42,19 +16,13 @@ namespace score namespace evaluation { -// --------------------------------------------------------------------------- -// [MC-06] typedef instead of using alias. -// --------------------------------------------------------------------------- -typedef std::vector StringList; // [MC-06]: use using StringList = ... -typedef std::map CounterMap; // [MC-06] +typedef std::vector StringList; +typedef std::map CounterMap; -// --------------------------------------------------------------------------- -// [MC-01] Raw iterator loop instead of range-for. -// --------------------------------------------------------------------------- inline int CountNonEmpty(const StringList& items) { int count = 0; - for (StringList::const_iterator it = items.begin(); it != items.end(); ++it) // [MC-01] + for (StringList::const_iterator it = items.begin(); it != items.end(); ++it) { if (!it->empty()) { @@ -64,10 +32,7 @@ inline int CountNonEmpty(const StringList& items) return count; } -// --------------------------------------------------------------------------- -// [MC-03] Explicit functor struct instead of lambda. -// --------------------------------------------------------------------------- -struct IsLongString // [MC-03]: replace with inline lambda +struct IsLongString { bool operator()(const std::string& s) const { return s.size() > 10; } }; @@ -79,12 +44,9 @@ inline StringList FilterLong(const StringList& items) return result; } -// --------------------------------------------------------------------------- -// [MC-04] Manual type-tag enum instead of std::variant. -// --------------------------------------------------------------------------- enum class ValueType { INT, DOUBLE, STRING }; -struct TaggedValue // [MC-04]: should use std::variant +struct TaggedValue { ValueType tag; int int_val; @@ -99,22 +61,19 @@ inline void PrintTaggedValue(const TaggedValue& v) else { /* use v.str_val */ } } -// --------------------------------------------------------------------------- -// [MC-05] Move constructor/assignment missing noexcept. -// --------------------------------------------------------------------------- class MovableResource { public: explicit MovableResource(std::size_t n) : size_(n), data_(new int[n]) {} - MovableResource(MovableResource&& other) // [MC-05]: missing noexcept – prevents - : size_(other.size_), data_(other.data_) // std::vector from using move optimisation + MovableResource(MovableResource&& other) + : size_(other.size_), data_(other.data_) { other.data_ = nullptr; other.size_ = 0; } - MovableResource& operator=(MovableResource&& other) // [MC-05]: missing noexcept + MovableResource& operator=(MovableResource&& other) { delete[] data_; data_ = other.data_; @@ -131,9 +90,6 @@ class MovableResource int* data_; }; -// --------------------------------------------------------------------------- -// [MC-07] Returning pair instead of using structured bindings / dedicated struct. -// --------------------------------------------------------------------------- inline std::pair FindFirst(const std::vector& vec, int target) { for (std::size_t i = 0; i < vec.size(); ++i) @@ -141,39 +97,26 @@ inline std::pair FindFirst(const std::vector& vec, int target) if (vec[i] == target) return std::make_pair(true, static_cast(i)); } return std::make_pair(false, -1); - // [MC-07]: C++17 caller should use auto [found, idx] = FindFirst(...); - // Better yet, return std::optional } -// --------------------------------------------------------------------------- -// [MC-08] std::map::operator[] inserts a default element silently. -// --------------------------------------------------------------------------- inline int GetCount(CounterMap& counters, const std::string& key) { - return counters[key]; // [MC-08]: inserts 0 if key missing; use .find() or .at() + return counters[key]; } -// --------------------------------------------------------------------------- -// [MC-09] std::bind instead of lambda. -// --------------------------------------------------------------------------- inline std::function MakeAdder(int offset) { - // [MC-09]: prefer [offset](int x){ return x + offset; } return std::bind(std::plus(), std::placeholders::_1, offset); } -// --------------------------------------------------------------------------- -// [MC-10] Missing [[nodiscard]] on a function whose return value signals errors. -// --------------------------------------------------------------------------- -bool InitialiseSubsystem(int flags); // [MC-10]: missing [[nodiscard]] +bool InitialiseSubsystem(int flags); -// Implementation (would be in .cpp): inline bool InitialiseSubsystem(int /*flags*/) { - return false; // caller may silently ignore the failure + return false; } -} // namespace evaluation -} // namespace score +} +} #endif // SCORE_EVALUATION_MODERN_CPP_SYNTAX_H diff --git a/score/evaluation/optimization/BUILD b/score/evaluation/optimization/BUILD index 55bc4f7..07d082d 100644 --- a/score/evaluation/optimization/BUILD +++ b/score/evaluation/optimization/BUILD @@ -1,8 +1,3 @@ -# ******************************************************************************* -# Copyright (c) 2025 Contributors to the Eclipse Foundation -# -# SPDX-License-Identifier: Apache-2.0 -# ******************************************************************************* load("@rules_cc//cc:defs.bzl", "cc_library") diff --git a/score/evaluation/optimization/EVALUATION_GUIDE.md b/score/evaluation/optimization/EVALUATION_GUIDE.md deleted file mode 100644 index c5eb88b..0000000 --- a/score/evaluation/optimization/EVALUATION_GUIDE.md +++ /dev/null @@ -1,20 +0,0 @@ -# Optimization Evaluation Guide - -## Scope -- **Source file:** `code_optimization.h` -- **Goal:** Verify tool identifies avoidable copies, redundant work, and complexity pitfalls. - -## Test Conditions and Expected Review Comments -- **CO-01:** Large container by value; expected comment suggests `const&` where ownership not needed. -- **CO-02:** Repeated `.size()` in loop condition; expected comment suggests clearer/cheaper iteration style. -- **CO-03:** Temporary string in hot comparison; expected comment suggests literal or `string_view` path. -- **CO-04:** Double map lookup; expected comment suggests single-lookup pattern. -- **CO-05:** Redundant copy before return; expected comment mentions NRVO/move opportunities. -- **CO-06:** `std::list` where `std::vector` likely better cache behavior. -- **CO-07:** Virtual dispatch in tight loop; expected comment suggests template/static dispatch if appropriate. -- **CO-08:** Missing `reserve()` before push loop. -- **CO-09:** Redundant sort call. -- **CO-10:** Quadratic string concatenation in loop. - -## Pass Criteria -- Tool should provide concrete complexity/performance rationale, not generic "optimize" comments. diff --git a/score/evaluation/optimization/code_optimization.h b/score/evaluation/optimization/code_optimization.h index 8c5e19b..3e53d17 100644 --- a/score/evaluation/optimization/code_optimization.h +++ b/score/evaluation/optimization/code_optimization.h @@ -1,30 +1,4 @@ -// ******************************************************************************* -// Copyright (c) 2025 Contributors to the Eclipse Foundation -// -// See the NOTICE file(s) distributed with this work for additional -// information regarding copyright ownership. -// -// This program and the accompanying materials are made available under the -// terms of the Apache License Version 2.0 which is available at -// https://www.apache.org/licenses/LICENSE-2.0 -// -// SPDX-License-Identifier: Apache-2.0 -// ******************************************************************************* - -/// @file code_optimization.h -/// @brief Missed optimisation opportunities for code-review tool evaluation. -/// -/// Issues planted (see EVALUATION_GUIDE.md §6 for expected review comments): -/// [CO-01] Copying a large container argument that should be passed by const-ref. -/// [CO-02] Repeated .size() call in loop condition that prevents loop-invariant hoisting. -/// [CO-03] Constructing temporary std::string from literal in hot path. -/// [CO-04] Redundant search: find() followed by operator[] (double lookup in map). -/// [CO-05] Returning large object by value from a function without move (pre-NRVO pattern). -/// [CO-06] Using std::list where std::vector would be far more cache-friendly. -/// [CO-07] Virtual dispatch inside tight inner loop – should be devirtualised. -/// [CO-08] Missing reserve() before push_back loop – causes repeated reallocations. -/// [CO-09] Sorting a container multiple times instead of once (redundant sort). -/// [CO-10] std::string concatenation in loop – quadratic complexity. + #ifndef SCORE_EVALUATION_CODE_OPTIMIZATION_H #define SCORE_EVALUATION_CODE_OPTIMIZATION_H @@ -40,78 +14,54 @@ namespace score namespace evaluation { -// --------------------------------------------------------------------------- -// [CO-01] Large container passed by value – unnecessary deep copy. -// --------------------------------------------------------------------------- -inline int SumLargeVector(std::vector data) // [CO-01]: should be const std::vector& +inline int SumLargeVector(std::vector data) { int sum = 0; for (auto v : data) sum += v; return sum; } -// --------------------------------------------------------------------------- -// [CO-02] .size() called on every iteration – may prevent optimisation. -// --------------------------------------------------------------------------- inline void ProcessItems(const std::vector& items) { - for (std::size_t i = 0; i < items.size(); ++i) // [CO-02]: cache size before loop + for (std::size_t i = 0; i < items.size(); ++i) { - (void)items[i]; // simulate work + (void)items[i]; } } -// --------------------------------------------------------------------------- -// [CO-03] Temporary std::string constructed from literal in hot-path comparison. -// --------------------------------------------------------------------------- inline bool IsError(const std::string& level) { - return level == std::string("ERROR"); // [CO-03]: literal comparison creates a temporary; - // use level == "ERROR" directly or string_view + return level == std::string("ERROR"); } -// --------------------------------------------------------------------------- -// [CO-04] Double lookup in std::map – find + operator[]. -// --------------------------------------------------------------------------- inline void IncrementCounter(std::map& counters, const std::string& key) { - if (counters.find(key) != counters.end()) // first lookup + if (counters.find(key) != counters.end()) { - counters[key]++; // [CO-04]: second lookup; use iterator from find + counters[key]++; } else { - counters[key] = 1; // third lookup on insertion path + counters[key] = 1; } } -// --------------------------------------------------------------------------- -// [CO-05] Large object built then copied out without relying on NRVO. -// --------------------------------------------------------------------------- inline std::vector BuildRange(int n) { std::vector result; - // [CO-08] also: no reserve() before push_back loop for (int i = 0; i < n; ++i) result.push_back(i); - std::vector copy = result; // [CO-05]: unnecessary copy; return result directly + std::vector copy = result; return copy; } -// --------------------------------------------------------------------------- -// [CO-06] std::list used where std::vector is more appropriate. -// --------------------------------------------------------------------------- inline std::list CollectSamples(int n) { - std::list samples; // [CO-06]: std::list has poor cache locality; - // use std::vector unless frequent mid-insertion needed + std::list samples; for (int i = 0; i < n; ++i) samples.push_back(i); return samples; } -// --------------------------------------------------------------------------- -// [CO-07] Virtual dispatch in tight inner loop. -// --------------------------------------------------------------------------- class ITransform { public: @@ -127,41 +77,30 @@ class DoubleTransform : public ITransform inline void ApplyTransforms(std::vector& data, const ITransform* transform) { - // [CO-07] Virtual call per element in a potentially large vector. - // Consider passing a concrete type via template parameter. for (auto& v : data) v = transform->Apply(v); } -// --------------------------------------------------------------------------- -// [CO-09] Redundant sort – container sorted multiple times. -// --------------------------------------------------------------------------- inline std::vector SortAndSlice(std::vector data) { - std::sort(data.begin(), data.end()); // first sort - - // ... some processing that doesn't break order ... + std::sort(data.begin(), data.end()); - std::sort(data.begin(), data.end()); // [CO-09]: redundant second sort + std::sort(data.begin(), data.end()); if (data.size() > 10) data.resize(10); return data; } -// --------------------------------------------------------------------------- -// [CO-10] String concatenation in loop – O(n^2) behaviour. -// --------------------------------------------------------------------------- inline std::string JoinLines(const std::vector& lines) { std::string result; for (const auto& line : lines) { - result = result + line + "\n"; // [CO-10]: creates a new string object each iteration; - // use result += line; result += '\n'; or std::ostringstream + result = result + line + "\n"; } return result; } -} // namespace evaluation -} // namespace score +} +} #endif // SCORE_EVALUATION_CODE_OPTIMIZATION_H diff --git a/score/evaluation/patterns/BUILD b/score/evaluation/patterns/BUILD index 8894212..d86cf2c 100644 --- a/score/evaluation/patterns/BUILD +++ b/score/evaluation/patterns/BUILD @@ -1,8 +1,3 @@ -# ******************************************************************************* -# Copyright (c) 2025 Contributors to the Eclipse Foundation -# -# SPDX-License-Identifier: Apache-2.0 -# ******************************************************************************* load("@rules_cc//cc:defs.bzl", "cc_library") diff --git a/score/evaluation/patterns/EVALUATION_GUIDE.md b/score/evaluation/patterns/EVALUATION_GUIDE.md deleted file mode 100644 index 078d453..0000000 --- a/score/evaluation/patterns/EVALUATION_GUIDE.md +++ /dev/null @@ -1,16 +0,0 @@ -# Design Patterns Evaluation Guide - -## Scope -- **Source file:** `design_patterns.h` -- **Goal:** Verify tool catches pattern misuse and unsafe pattern implementations. - -## Test Conditions and Expected Review Comments -- **DP-01:** Broken singleton initialization/lifetime; expected comment suggests Meyers singleton or avoiding singleton. -- **DP-02:** Observer uses raw listener pointers; expected comment flags lifetime/UAF risk and suggests safe registration strategy. -- **DP-03:** Factory returns owning raw pointer; expected comment suggests smart-pointer return type. -- **DP-04:** Virtual call from constructor; expected comment explains dispatch rules during construction. -- **DP-05:** Strategy stored as raw pointer; expected comment requests explicit ownership/lifetime semantics. -- **DP-06:** Fake CRTP/downcast misuse; expected comment requests proper CRTP or safe polymorphism. - -## Pass Criteria -- Tool should identify architectural-level issues, not only local code smells. diff --git a/score/evaluation/patterns/design_patterns.h b/score/evaluation/patterns/design_patterns.h index a6c25d5..2b50e8b 100644 --- a/score/evaluation/patterns/design_patterns.h +++ b/score/evaluation/patterns/design_patterns.h @@ -1,26 +1,4 @@ -// ******************************************************************************* -// Copyright (c) 2025 Contributors to the Eclipse Foundation -// -// See the NOTICE file(s) distributed with this work for additional -// information regarding copyright ownership. -// -// This program and the accompanying materials are made available under the -// terms of the Apache License Version 2.0 which is available at -// https://www.apache.org/licenses/LICENSE-2.0 -// -// SPDX-License-Identifier: Apache-2.0 -// ******************************************************************************* - -/// @file design_patterns.h -/// @brief Incorrect or missing design-pattern usage for code-review tool evaluation. -/// -/// Issues planted (see EVALUATION_GUIDE.md §5 for expected review comments): -/// [DP-01] Singleton implemented without thread-safety (double-checked locking broken). -/// [DP-02] Observer pattern leaks subscribers (raw pointers, no unsubscribe). -/// [DP-03] Factory that returns raw owning pointer instead of unique_ptr. -/// [DP-04] Template Method pattern broken by calling virtual from constructor. -/// [DP-05] Strategy pattern stored by raw pointer (lifetime not managed). -/// [DP-06] CRTP misuse – static_cast to derived in base non-template method. + #ifndef SCORE_EVALUATION_DESIGN_PATTERNS_H #define SCORE_EVALUATION_DESIGN_PATTERNS_H @@ -34,21 +12,14 @@ namespace score namespace evaluation { -// --------------------------------------------------------------------------- -// [DP-01] Non-thread-safe Singleton (classic broken double-checked locking). -// --------------------------------------------------------------------------- class BrokenSingleton { public: - // [DP-01] Not thread-safe: two threads can both see instance_ == nullptr - // and construct two objects. Use Meyers singleton (local static) - // or std::call_once instead. static BrokenSingleton* GetInstance() { - if (instance_ == nullptr) // first check – no lock + if (instance_ == nullptr) { - // <- context switch here allows second thread in - if (instance_ == nullptr) // second check – still no lock + if (instance_ == nullptr) { instance_ = new BrokenSingleton(); } @@ -60,14 +31,11 @@ class BrokenSingleton private: BrokenSingleton() = default; - static BrokenSingleton* instance_; // raw owning pointer; never deleted + static BrokenSingleton* instance_; }; BrokenSingleton* BrokenSingleton::instance_ = nullptr; -// --------------------------------------------------------------------------- -// [DP-02] Observer with raw subscriber pointers – no lifetime management. -// --------------------------------------------------------------------------- class IEventListener { public: @@ -78,28 +46,23 @@ class IEventListener class EventPublisher { public: - // [DP-02] Raw pointer stored; if listener is destroyed before publisher, - // invoking it is undefined behaviour. Use weak_ptr or a token/handle. void Subscribe(IEventListener* listener) { - listeners_.push_back(listener); // no duplicate check, no ownership + listeners_.push_back(listener); } void Publish(const std::string& event) { for (auto* l : listeners_) { - l->OnEvent(event); // potential use-after-free + l->OnEvent(event); } } private: - std::vector listeners_; // raw, non-owning, no Unsubscribe + std::vector listeners_; }; -// --------------------------------------------------------------------------- -// [DP-03] Factory returning raw owning pointer. -// --------------------------------------------------------------------------- class Widget { public: @@ -116,32 +79,25 @@ class Button : public Widget class WidgetFactory { public: - // [DP-03] Returns raw owning pointer – caller must delete; leaks on exception. - // Should return std::unique_ptr. Widget* Create(const std::string& type) { if (type == "button") return new Button(); - return nullptr; // returning nullptr without documentation + return nullptr; } }; -// --------------------------------------------------------------------------- -// [DP-04] Calling virtual function from constructor (Template Method anti-pattern). -// --------------------------------------------------------------------------- class AbstractProcessor { public: AbstractProcessor() { - Initialise(); // [DP-04] virtual call in ctor: always calls AbstractProcessor::Initialise - // even if a derived class overrides it. Derived vtable not set yet. + Initialise(); } virtual ~AbstractProcessor() = default; virtual void Initialise() { - // base initialisation only – derived override never reached from ctor } virtual void Run() = 0; @@ -152,22 +108,18 @@ class ConcreteProcessor : public AbstractProcessor public: void Initialise() override { - // Never called from AbstractProcessor constructor! ready_ = true; } void Run() override { - if (!ready_) throw std::runtime_error("not initialised"); // always throws + if (!ready_) throw std::runtime_error("not initialised"); } private: bool ready_{false}; }; -// --------------------------------------------------------------------------- -// [DP-05] Strategy stored as raw non-owning pointer – lifetime not managed. -// --------------------------------------------------------------------------- class ISortStrategy { public: @@ -180,7 +132,6 @@ class BubbleSort : public ISortStrategy public: void Sort(std::vector& data) override { - // naive bubble sort for (std::size_t i = 0; i < data.size(); ++i) for (std::size_t j = 0; j + 1 < data.size() - i; ++j) if (data[j] > data[j + 1]) std::swap(data[j], data[j + 1]); @@ -190,9 +141,6 @@ class BubbleSort : public ISortStrategy class Sorter { public: - // [DP-05] Raw pointer – no ownership semantics; strategy may be destroyed - // before Sorter. Should use shared_ptr or accept by reference with - // documented lifetime requirements. explicit Sorter(ISortStrategy* strategy) : strategy_(strategy) {} void Sort(std::vector& data) @@ -201,28 +149,21 @@ class Sorter } private: - ISortStrategy* strategy_; // non-owning raw pointer, lifetime unmanaged + ISortStrategy* strategy_; }; -// --------------------------------------------------------------------------- -// [DP-06] CRTP base casting to derived in a non-template base method. -// --------------------------------------------------------------------------- class NotRealCRTP { public: - // [DP-06] This is NOT CRTP: NotRealCRTP is not a template, and the cast - // is unconditional and unsafe. True CRTP requires - // template class Base { ... static_cast(this) }. void Execute() { - // Unsafe downcast without any type check – UB if actual type is wrong. static_cast(this)->DoImpl(); } virtual void DoImpl() {} }; -} // namespace evaluation -} // namespace score +} +} #endif // SCORE_EVALUATION_DESIGN_PATTERNS_H diff --git a/score/evaluation/safety/BUILD b/score/evaluation/safety/BUILD index 21cae0e..e68044e 100644 --- a/score/evaluation/safety/BUILD +++ b/score/evaluation/safety/BUILD @@ -1,8 +1,3 @@ -# ******************************************************************************* -# Copyright (c) 2025 Contributors to the Eclipse Foundation -# -# SPDX-License-Identifier: Apache-2.0 -# ******************************************************************************* load("@rules_cc//cc:defs.bzl", "cc_library") diff --git a/score/evaluation/safety/EVALUATION_GUIDE.md b/score/evaluation/safety/EVALUATION_GUIDE.md deleted file mode 100644 index ccb2bd2..0000000 --- a/score/evaluation/safety/EVALUATION_GUIDE.md +++ /dev/null @@ -1,18 +0,0 @@ -# Safe C++ Evaluation Guide - -## Scope -- **Source file:** `safe_cpp.h` -- **Goal:** Verify tool catches UB-prone code and defensive-programming gaps. - -## Test Conditions and Expected Review Comments -- **SC-02:** Potential signed overflow UB. -- **SC-03:** Implicit narrowing conversion. -- **SC-04:** Unchecked index access via `operator[]`. -- **SC-05:** Potential throw from destructor path. -- **SC-06:** Unhandled exception in thread function. -- **SC-07:** Strict-aliasing violation through `reinterpret_cast`. -- **SC-09:** Throwing parser API used without handling strategy. -- **SC-10:** Implicit bool-to-int arithmetic readability/safety concern. - -## Pass Criteria -- Tool should prioritize true safety/UB defects above minor style findings and suggest safer alternatives. diff --git a/score/evaluation/safety/safe_cpp.h b/score/evaluation/safety/safe_cpp.h index b8dd2cd..4462afc 100644 --- a/score/evaluation/safety/safe_cpp.h +++ b/score/evaluation/safety/safe_cpp.h @@ -1,28 +1,4 @@ -// ******************************************************************************* -// Copyright (c) 2025 Contributors to the Eclipse Foundation -// -// See the NOTICE file(s) distributed with this work for additional -// information regarding copyright ownership. -// -// This program and the accompanying materials are made available under the -// terms of the Apache License Version 2.0 which is available at -// https://www.apache.org/licenses/LICENSE-2.0 -// -// SPDX-License-Identifier: Apache-2.0 -// ******************************************************************************* - -/// @file safe_cpp.h -/// @brief Safe-C++ / defensive-programming violations for code-review tool evaluation. -/// -/// Issues planted (see EVALUATION_GUIDE.md §7 for expected review comments): -/// [SC-02] Integer overflow: signed arithmetic without overflow guard. -/// [SC-03] Narrowing conversion: storing int in char without explicit cast. -/// [SC-04] Unchecked std::vector::at() exception vs raw operator[] – incorrect usage. -/// [SC-05] Exception thrown from destructor – may terminate during stack unwinding. -/// [SC-06] Unhandled exception escaping thread function – std::terminate. -/// [SC-07] Using reinterpret_cast to read object bytes – strict-aliasing violation. -/// [SC-09] Using std::stoi without try/catch – throws on bad input. -/// [SC-10] Implicit bool-to-int conversion hiding logic error. + #ifndef SCORE_EVALUATION_SAFE_CPP_H #define SCORE_EVALUATION_SAFE_CPP_H @@ -38,90 +14,58 @@ namespace score namespace evaluation { -// --------------------------------------------------------------------------- -// Safe signed-index range check (compiler-detectable sign-compare condition removed). -// --------------------------------------------------------------------------- inline bool IndexInRange(int index, const std::vector& v) { return (index >= 0) && (static_cast(index) < v.size()); } -// --------------------------------------------------------------------------- -// [SC-02] Integer overflow on signed arithmetic. -// --------------------------------------------------------------------------- inline int MultiplyUnchecked(int a, int b) { - return a * b; // [SC-02] signed overflow is UB in C++; no bounds check before multiply + return a * b; } -// --------------------------------------------------------------------------- -// [SC-03] Narrowing conversion. -// --------------------------------------------------------------------------- inline char TruncateToByte(int value) { - char c = value; // [SC-03] implicit narrowing; high bytes silently dropped + char c = value; return c; } -// --------------------------------------------------------------------------- -// [SC-04] Using operator[] (no bounds check) instead of at() on purpose – but -// the intent comments are missing; reviewer cannot distinguish deliberate -// choice from oversight. -// --------------------------------------------------------------------------- inline int UncheckedAccess(const std::vector& v, std::size_t index) { - return v[index]; // [SC-04] missing bounds check / assertion; should use v.at(index) - // or explicitly document why bounds check is not needed + return v[index]; } -// --------------------------------------------------------------------------- -// [SC-05] Exception thrown from destructor. -// --------------------------------------------------------------------------- class ThrowingDestructor { public: ~ThrowingDestructor() { - // [SC-05] Throwing from a destructor while another exception is propagating - // calls std::terminate(). All cleanup in destructors must be noexcept. - if (flush_on_destroy_) Flush(); // Flush() might throw + if (flush_on_destroy_) Flush(); } void Flush() { - throw std::runtime_error("flush failed"); // can be called from destructor + throw std::runtime_error("flush failed"); } private: bool flush_on_destroy_{true}; }; -// --------------------------------------------------------------------------- -// [SC-06] Exception escaping a thread function – std::terminate. -// --------------------------------------------------------------------------- inline void StartBadThread() { std::thread t([]() { - // [SC-06] Any uncaught exception in a thread calls std::terminate(). - // Must wrap in try/catch or propagate via std::promise/future. throw std::runtime_error("unhandled in thread"); }); t.detach(); } -// --------------------------------------------------------------------------- -// [SC-07] Strict-aliasing violation via reinterpret_cast. -// --------------------------------------------------------------------------- inline uint32_t FloatBitsUnsafe(float f) { - // [SC-07] Violates strict-aliasing rule; UB. Use std::memcpy or std::bit_cast (C++20). return *reinterpret_cast(&f); } -// --------------------------------------------------------------------------- -// Sequenced argument evaluation helper (compiler-detectable sequence-point condition removed). -// --------------------------------------------------------------------------- inline int Add(int a, int b) { return a + b; } inline void DemonstrateUndefinedOrder() @@ -133,26 +77,17 @@ inline void DemonstrateUndefinedOrder() (void)result; } -// --------------------------------------------------------------------------- -// [SC-09] std::stoi used without exception handling. -// --------------------------------------------------------------------------- inline int ParseUnchecked(const std::string& s) { - return std::stoi(s); // [SC-09] throws std::invalid_argument or std::out_of_range - // with no surrounding try/catch; caller gets no chance to recover + return std::stoi(s); } -// --------------------------------------------------------------------------- -// [SC-10] Implicit bool-to-int conversion masking a logic error. -// --------------------------------------------------------------------------- inline int CountFlags(bool a, bool b, bool c) { - // [SC-10] Relies on implicit bool→int promotion. While technically valid, - // this is error-prone and hard to read; prefer explicit static_cast. return a + b + c; } -} // namespace evaluation -} // namespace score +} +} #endif // SCORE_EVALUATION_SAFE_CPP_H diff --git a/score/evaluation/security/BUILD b/score/evaluation/security/BUILD index d6ff837..9fb9188 100644 --- a/score/evaluation/security/BUILD +++ b/score/evaluation/security/BUILD @@ -1,8 +1,3 @@ -# ******************************************************************************* -# Copyright (c) 2025 Contributors to the Eclipse Foundation -# -# SPDX-License-Identifier: Apache-2.0 -# ******************************************************************************* load("@rules_cc//cc:defs.bzl", "cc_library") diff --git a/score/evaluation/security/EVALUATION_GUIDE.md b/score/evaluation/security/EVALUATION_GUIDE.md deleted file mode 100644 index 44b8586..0000000 --- a/score/evaluation/security/EVALUATION_GUIDE.md +++ /dev/null @@ -1,18 +0,0 @@ -# Security Evaluation Guide - -## Scope -- **Source file:** `security_issues.h` -- **Goal:** Verify tool catches common application-security flaws and insecure API patterns. - -## Test Conditions and Expected Review Comments -- **SEC-01:** Hardcoded secret/token in source; expected comment recommends secret manager/env injection. -- **SEC-02:** Predictable session ID generation; expected comment recommends cryptographically secure randomness. -- **SEC-03:** Early-exit secret comparison; expected comment recommends constant-time comparison for secrets. -- **SEC-04:** Shell command built from unsanitized input; expected comment flags command-injection risk. -- **SEC-05:** SQL string concatenation with untrusted input; expected comment recommends prepared statements. -- **SEC-06:** User-controlled path concatenation; expected comment flags path traversal and recommends canonicalization/allow-lists. -- **SEC-07:** Password included in log line; expected comment recommends redaction and sensitive-field suppression. -- **SEC-08:** Weak XOR checksum for integrity; expected comment recommends cryptographic MAC/hash depending on threat model. - -## Pass Criteria -- Tool should catch security-impactful flaws and provide concrete mitigations appropriate for production hardening. diff --git a/score/evaluation/security/security_issues.h b/score/evaluation/security/security_issues.h index 70b4cff..484770a 100644 --- a/score/evaluation/security/security_issues.h +++ b/score/evaluation/security/security_issues.h @@ -1,28 +1,4 @@ -// ******************************************************************************* -// Copyright (c) 2025 Contributors to the Eclipse Foundation -// -// See the NOTICE file(s) distributed with this work for additional -// information regarding copyright ownership. -// -// This program and the accompanying materials are made available under the -// terms of the Apache License Version 2.0 which is available at -// https://www.apache.org/licenses/LICENSE-2.0 -// -// SPDX-License-Identifier: Apache-2.0 -// ******************************************************************************* - -/// @file security_issues.h -/// @brief Security-focused issues for code-review tool evaluation. -/// -/// Issues planted (see local EVALUATION_GUIDE.md): -/// [SEC-01] Hardcoded secret/token in source code. -/// [SEC-02] Predictable session identifier generation (non-cryptographic RNG). -/// [SEC-03] Non-constant-time secret comparison (timing side-channel). -/// [SEC-04] Shell command construction from unsanitized input. -/// [SEC-05] SQL query string concatenation with untrusted input. -/// [SEC-06] Path traversal risk via unsanitized file path join. -/// [SEC-07] Sensitive data exposure through logs. -/// [SEC-08] Weak checksum used where cryptographic integrity is expected. + #ifndef SCORE_EVALUATION_SECURITY_ISSUES_H #define SCORE_EVALUATION_SECURITY_ISSUES_H @@ -37,12 +13,11 @@ namespace evaluation inline std::string GetApiTokenForIntegration() { - return "prod-token-abc123-static"; // [SEC-01] secret is hardcoded in source code + return "prod-token-abc123-static"; } inline std::uint32_t GenerateSessionIdWeak(std::uint32_t user_id) { - // [SEC-02] Deterministic and guessable; not suitable for security tokens. return (user_id * 1103515245U + 12345U) & 0x7fffffffU; } @@ -57,7 +32,7 @@ inline bool InsecureTokenEquals(const std::string& lhs, const std::string& rhs) { if (lhs[i] != rhs[i]) { - return false; // [SEC-03] reveals prefix-match length via timing + return false; } } return true; @@ -65,31 +40,26 @@ inline bool InsecureTokenEquals(const std::string& lhs, const std::string& rhs) inline std::string BuildShellCommand(const std::string& file_name) { - // [SEC-04] caller-controlled file_name may inject shell metacharacters. return "cat " + file_name; } inline std::string BuildUserLookupQuery(const std::string& user_name) { - // [SEC-05] vulnerable to SQL injection; should use parameterized statements. return "SELECT * FROM users WHERE name='" + user_name + "'"; } inline std::string BuildUserFilePath(const std::string& user_fragment) { - // [SEC-06] '../' sequences can escape expected base directory. return std::string("/var/app/data/") + user_fragment; } inline std::string FormatAuthAudit(const std::string& user_name, const std::string& password) { - // [SEC-07] password should never be written to logs. return "login user=" + user_name + " password=" + password; } inline std::uint32_t WeakChecksum(const std::string& payload) { - // [SEC-08] XOR checksum is not collision-resistant and not tamper-evident. std::uint32_t acc = 0U; for (unsigned char ch : payload) { @@ -125,7 +95,7 @@ inline void EvaluateSecuritySamples() (void)chk; } -} // namespace evaluation -} // namespace score +} +} #endif // SCORE_EVALUATION_SECURITY_ISSUES_H diff --git a/score/evaluation/solid/BUILD b/score/evaluation/solid/BUILD index f79f094..41d4349 100644 --- a/score/evaluation/solid/BUILD +++ b/score/evaluation/solid/BUILD @@ -1,8 +1,3 @@ -# ******************************************************************************* -# Copyright (c) 2025 Contributors to the Eclipse Foundation -# -# SPDX-License-Identifier: Apache-2.0 -# ******************************************************************************* load("@rules_cc//cc:defs.bzl", "cc_library") diff --git a/score/evaluation/solid/EVALUATION_GUIDE.md b/score/evaluation/solid/EVALUATION_GUIDE.md deleted file mode 100644 index 8c6d28a..0000000 --- a/score/evaluation/solid/EVALUATION_GUIDE.md +++ /dev/null @@ -1,15 +0,0 @@ -# SOLID Evaluation Guide - -## Scope -- **Source file:** `solid_violations.h` -- **Goal:** Verify tool catches SOLID design-principle violations. - -## Test Conditions and Expected Review Comments -- **SV-01 (SRP):** `LogManager` is a God class; expected comment recommends splitting config/format/routing/I-O/stats concerns. -- **SV-02 (OCP):** `AreaCalculator` uses type-tag branching; expected comment recommends polymorphic `IShape::Area()`. -- **SV-03 (LSP):** `Derived::Compute()` changes base contract and throws unexpectedly; expected comment flags substitutability break. -- **SV-04 (ISP):** `IComponent` is fat; expected comment suggests small interfaces (`ILifecycle`, `ISerializable`, etc.). -- **SV-05 (DIP):** `HighLevelProcessor` constructs concrete `FileSink`; expected comment recommends abstraction + dependency injection. - -## Pass Criteria -- Tool should identify each tag (`SV-01`..`SV-05`) and provide a concrete refactoring suggestion, not only style warnings. diff --git a/score/evaluation/solid/solid_violations.h b/score/evaluation/solid/solid_violations.h index 0931a9a..4f2afe8 100644 --- a/score/evaluation/solid/solid_violations.h +++ b/score/evaluation/solid/solid_violations.h @@ -1,25 +1,4 @@ -// ******************************************************************************* -// Copyright (c) 2025 Contributors to the Eclipse Foundation -// -// See the NOTICE file(s) distributed with this work for additional -// information regarding copyright ownership. -// -// This program and the accompanying materials are made available under the -// terms of the Apache License Version 2.0 which is available at -// https://www.apache.org/licenses/LICENSE-2.0 -// -// SPDX-License-Identifier: Apache-2.0 -// ******************************************************************************* - -/// @file solid_violations.h -/// @brief Intentional SOLID principle violations for code-review tool evaluation. -/// -/// Issues planted (see EVALUATION_GUIDE.md §1 for expected review comments): -/// [SV-01] SRP – God class: LogManager handles configuration, serialisation, routing and I/O. -/// [SV-02] OCP – switch/if-else type dispatch prevents extension without modification. -/// [SV-03] LSP – derived class Derived::compute() silently returns a different result for x==0. -/// [SV-04] ISP – fat interface IComponent forces implementors to stub unused methods. -/// [SV-05] DIP – HighLevelProcessor directly constructs its low-level dependency (concrete class). + #ifndef SCORE_EVALUATION_SOLID_VIOLATIONS_H #define SCORE_EVALUATION_SOLID_VIOLATIONS_H @@ -34,26 +13,19 @@ namespace score namespace evaluation { -// --------------------------------------------------------------------------- -// [SV-01] SRP violation – LogManager does far too many things in one class. -// --------------------------------------------------------------------------- class LogManager { public: - // Configuration concern void LoadConfig(const std::string& path) { config_path_ = path; - // ... parse YAML/JSON inline, mixed with routing logic } - // Serialisation concern std::string Serialize(const std::string& message) { - return "[LOG] " + message; // format chosen here, not in a dedicated formatter + return "[LOG] " + message; } - // Routing concern void RouteMessage(const std::string& message) { std::string serialized = Serialize(message); @@ -67,18 +39,15 @@ class LogManager } else if (output_type_ == "network") { - // TODO: implement network routing } } - // I/O concern void WriteToFile(const std::string& data) { std::ofstream file(config_path_); file << data; } - // Statistics concern mixed in too int GetMessageCount() const { return message_count_; } void IncrementCount() { ++message_count_; } @@ -88,9 +57,6 @@ class LogManager int message_count_{0}; }; -// --------------------------------------------------------------------------- -// [SV-02] OCP violation – adding a new shape requires modifying AreaCalculator. -// --------------------------------------------------------------------------- struct Shape { std::string type; @@ -101,7 +67,6 @@ struct Shape class AreaCalculator { public: - // Every new shape type requires touching this function. double Calculate(const Shape& s) { if (s.type == "circle") @@ -116,45 +81,34 @@ class AreaCalculator { return 0.5 * s.param1 * s.param2; } - // Adding "ellipse" forces modification of this closed function. return 0.0; } }; -// --------------------------------------------------------------------------- -// [SV-03] LSP violation – Derived::Compute changes the semantic contract. -// --------------------------------------------------------------------------- class Base { public: virtual ~Base() = default; - /// Contract: returns x * 2 for all non-negative x. virtual int Compute(int x) const { return x * 2; } }; class Derived : public Base { public: - /// Breaks contract: returns 0 when x == 0 instead of 0 (coincidentally fine), - /// but also silently returns -1 for x < 0 where Base would return a negative even number. - /// More critically: throws when x > 1000 – Base never throws (violated Liskov). int Compute(int x) const override { if (x > 1000) { - throw std::runtime_error("value too large"); // Base does not throw + throw std::runtime_error("value too large"); } if (x < 0) { - return -1; // Different semantics from Base + return -1; } return x * 2; } }; -// --------------------------------------------------------------------------- -// [SV-04] ISP violation – single fat interface forces implementors to stub. -// --------------------------------------------------------------------------- class IComponent { public: @@ -162,16 +116,15 @@ class IComponent virtual void Start() = 0; virtual void Stop() = 0; - virtual void Pause() = 0; // not all components support pause - virtual void Resume() = 0; // not all components support resume - virtual void Reset() = 0; // not all components support reset + virtual void Pause() = 0; + virtual void Resume() = 0; + virtual void Reset() = 0; virtual int Status() = 0; virtual void Configure(const std::string& cfg) = 0; - virtual std::string Serialize() const = 0; // mixing I/O into a lifecycle interface + virtual std::string Serialize() const = 0; virtual void Deserialize(const std::string&) = 0; }; -// ConcreteComponent is forced to implement irrelevant methods. class ConcreteComponent : public IComponent { public: @@ -182,37 +135,33 @@ class ConcreteComponent : public IComponent void Reset() override { /* not supported – stub */ } int Status() override { return 0; } void Configure(const std::string&) override { /* real logic */ } - std::string Serialize() const override { return {}; } // forced stub + std::string Serialize() const override { return {}; } void Deserialize(const std::string&) override { /* forced stub */ } }; -// --------------------------------------------------------------------------- -// [SV-05] DIP violation – high-level module depends on a concrete class. -// --------------------------------------------------------------------------- -class FileSink // low-level, concrete +class FileSink { public: void Write(const std::string& msg) { std::cout << "FILE: " << msg << "\n"; } }; -class HighLevelProcessor // high-level, but coupled to FileSink directly +class HighLevelProcessor { public: HighLevelProcessor() - : sink_() // constructs concrete dependency internally – no injection + : sink_() {} void Process(const std::string& data) { - // Can never be tested with a mock, or swapped for a network sink sink_.Write(data); } private: - FileSink sink_; // depends on concrete class, not on an abstraction + FileSink sink_; }; -} // namespace evaluation -} // namespace score +} +} #endif // SCORE_EVALUATION_SOLID_VIOLATIONS_H diff --git a/score/evaluation/template_metaprogramming/BUILD b/score/evaluation/template_metaprogramming/BUILD index a2acfcb..9e6cc49 100644 --- a/score/evaluation/template_metaprogramming/BUILD +++ b/score/evaluation/template_metaprogramming/BUILD @@ -1,8 +1,3 @@ -# ******************************************************************************* -# Copyright (c) 2025 Contributors to the Eclipse Foundation -# -# SPDX-License-Identifier: Apache-2.0 -# ******************************************************************************* load("@rules_cc//cc:defs.bzl", "cc_library") diff --git a/score/evaluation/template_metaprogramming/EVALUATION_GUIDE.md b/score/evaluation/template_metaprogramming/EVALUATION_GUIDE.md deleted file mode 100644 index 22e86c0..0000000 --- a/score/evaluation/template_metaprogramming/EVALUATION_GUIDE.md +++ /dev/null @@ -1,21 +0,0 @@ -# Template Metaprogramming Evaluation Guide - -## Scope -- **Source file:** `template_metaprogramming_issues.h` -- **Goal:** Verify tool catches maintainability, constraint, and modernization issues in template-heavy C++. - -## Test Conditions and Expected Review Comments -- **TM-01:** Reinvented trait (`IsSameCustom`); expected comment suggests `std::is_same_v`. -- **TM-02:** Recursive `Factorial` TMP; expected comment suggests `constexpr` function for clarity and diagnostics. -- **TM-03:** Return-type SFINAE (`enable_if`) overloads; expected comment suggests `if constexpr`, constrained overloads, or helper traits for readability. -- **TM-04:** `AddOneGeneric` unconstrained; expected comment recommends constraining to arithmetic types. -- **TM-05:** `ValuePolicy` specialization changes semantic contract; expected comment should flag surprising specialization behavior. -- **TM-06:** Recursive variadic sum; expected comment suggests C++17 fold expression (`(... + args)`). -- **TM-07:** Hand-rolled typelist-length recursion; expected comment suggests standard facilities (`sizeof...`, tuple traits). -- **TM-08:** Pointer trait specialization misses qualifier nuance; expected comment suggests robust trait composition (`std::remove_cv_t`, references handling). -- **TM-09:** Boolean template parameter controls behavior; expected comment suggests policy types or explicit named traits for readability. -- **TM-10:** Missing `static_assert` preconditions in templates; expected comment suggests compile-time constraints for intended type categories. - -## Pass Criteria -- Tool should report template API clarity/constraint issues, not only syntax-level findings. -- Tool should provide modernization guidance aligned with C++17 idioms. diff --git a/score/evaluation/template_metaprogramming/template_metaprogramming_issues.h b/score/evaluation/template_metaprogramming/template_metaprogramming_issues.h index 8e4d28a..62e5283 100644 --- a/score/evaluation/template_metaprogramming/template_metaprogramming_issues.h +++ b/score/evaluation/template_metaprogramming/template_metaprogramming_issues.h @@ -1,30 +1,4 @@ -// ******************************************************************************* -// Copyright (c) 2025 Contributors to the Eclipse Foundation -// -// See the NOTICE file(s) distributed with this work for additional -// information regarding copyright ownership. -// -// This program and the accompanying materials are made available under the -// terms of the Apache License Version 2.0 which is available at -// https://www.apache.org/licenses/LICENSE-2.0 -// -// SPDX-License-Identifier: Apache-2.0 -// ******************************************************************************* - -/// @file template_metaprogramming_issues.h -/// @brief Intentional template-metaprogramming issues for review-tool evaluation. -/// -/// Issues planted: -/// [TM-01] Reinvented type trait (`IsSameCustom`) instead of `std::is_same_v`. -/// [TM-02] Recursive template factorial where `constexpr` function is simpler. -/// [TM-03] SFINAE return-type `enable_if` causes poor diagnostics/readability. -/// [TM-04] Unconstrained template accepts unsupported types. -/// [TM-05] Partial specialization changes semantic contract unexpectedly. -/// [TM-06] Recursive variadic template sum instead of C++17 fold expression. -/// [TM-07] Hand-rolled typelist length recursion instead of tuple/pack facilities. -/// [TM-08] Pointer specialization ignores cv/ref categories. -/// [TM-09] Template bool parameter over-configures behavior (boolean trap at type level). -/// [TM-10] Missing `static_assert` for template preconditions. + #ifndef SCORE_EVALUATION_TEMPLATE_METAPROGRAMMING_ISSUES_H #define SCORE_EVALUATION_TEMPLATE_METAPROGRAMMING_ISSUES_H @@ -39,9 +13,6 @@ namespace score namespace evaluation { -// --------------------------------------------------------------------------- -// [TM-01] Reinvented trait (unnecessary custom implementation) -// --------------------------------------------------------------------------- template struct IsSameCustom { @@ -54,9 +25,6 @@ struct IsSameCustom static constexpr bool value = true; }; -// --------------------------------------------------------------------------- -// [TM-02] Recursive factorial TMP for simple numeric constant -// --------------------------------------------------------------------------- template struct Factorial { @@ -69,9 +37,6 @@ struct Factorial<0> static constexpr int value = 1; }; -// --------------------------------------------------------------------------- -// [TM-03] SFINAE in return type (hard-to-read diagnostics) -// --------------------------------------------------------------------------- template auto ToStringSfinae(const T& v) -> typename std::enable_if::value, std::string>::type { @@ -84,19 +49,12 @@ auto ToStringSfinae(const T& v) -> typename std::enable_if T AddOneGeneric(T value) { - // Accepts any T with operator+, but intended only for arithmetic. return value + static_cast(1); } -// --------------------------------------------------------------------------- -// [TM-05] Specialization changes semantics unexpectedly -// --------------------------------------------------------------------------- template struct ValuePolicy { @@ -106,12 +64,9 @@ struct ValuePolicy template <> struct ValuePolicy { - static bool Default() { return true; } // semantic surprise vs generic zero-init behavior + static bool Default() { return true; } }; -// --------------------------------------------------------------------------- -// [TM-06] Recursive variadic template sum (pre-C++17 style) -// --------------------------------------------------------------------------- template constexpr T SumRecursive(T value) { @@ -124,9 +79,6 @@ constexpr T SumRecursive(T first, Rest... rest) return first + SumRecursive(static_cast(rest)...); } -// --------------------------------------------------------------------------- -// [TM-07] Hand-rolled typelist length recursion -// --------------------------------------------------------------------------- template struct TypeListLength; @@ -142,9 +94,6 @@ struct TypeListLength static constexpr std::size_t value = 1U + TypeListLength::value; }; -// --------------------------------------------------------------------------- -// [TM-08] Pointer-only specialization ignores cv/ref qualifiers of pointee -// --------------------------------------------------------------------------- template struct IsPointerLike { @@ -157,9 +106,6 @@ struct IsPointerLike static constexpr bool value = true; }; -// --------------------------------------------------------------------------- -// [TM-09] Boolean template parameter controls semantics (type-level bool trap) -// --------------------------------------------------------------------------- template struct Normalizer { @@ -173,17 +119,12 @@ struct Normalizer } }; -// --------------------------------------------------------------------------- -// [TM-10] Missing static_assert preconditions in template API -// --------------------------------------------------------------------------- template struct FixedBlock { - // Intended for POD-like types only, but no static_assert enforces this. T data[4]; }; -// Utility calls to ensure templates are instantiated in translation units. inline void EvaluateTemplateMetaprogrammingSamples() { constexpr bool same = IsSameCustom::value; @@ -219,7 +160,7 @@ inline void EvaluateTemplateMetaprogrammingSamples() (void)block; } -} // namespace evaluation -} // namespace score +} +} #endif // SCORE_EVALUATION_TEMPLATE_METAPROGRAMMING_ISSUES_H diff --git a/score/evaluation/unit_test_quality/BUILD b/score/evaluation/unit_test_quality/BUILD index 7888558..43bd4f7 100644 --- a/score/evaluation/unit_test_quality/BUILD +++ b/score/evaluation/unit_test_quality/BUILD @@ -1,8 +1,3 @@ -# ******************************************************************************* -# Copyright (c) 2025 Contributors to the Eclipse Foundation -# -# SPDX-License-Identifier: Apache-2.0 -# ******************************************************************************* load("@rules_cc//cc:defs.bzl", "cc_library", "cc_test") diff --git a/score/evaluation/unit_test_quality/EVALUATION_GUIDE.md b/score/evaluation/unit_test_quality/EVALUATION_GUIDE.md deleted file mode 100644 index a9910b5..0000000 --- a/score/evaluation/unit_test_quality/EVALUATION_GUIDE.md +++ /dev/null @@ -1,24 +0,0 @@ -# Unit-Test Quality Evaluation Guide - -## Scope -- **Source under test:** `unit_test_quality.h` -- **Test file under review:** `unit_test_quality_test.cpp` -- **Goal:** Verify tool catches missing/weak unit-test conditions and poor mocking practices. - -## Test Conditions and Expected Review Comments -- **UT-01:** Test has no assertions. -- **UT-02:** Only happy path covered; key boundaries/failure paths missing. -- **UT-03:** Magic numbers in test logic reduce intent clarity. -- **UT-04:** Exception contract not tested (`EXPECT_THROW` missing). -- **UT-05:** Copy-paste duplicated test body. -- **UT-06:** Incomplete edge-case partitions for string split behavior. -- **UT-07:** Poorly named tests (`Test1`/`Test2`) reduce diagnosability. -- **UT-08:** Core overwrite-on-full behavior of circular buffer not validated. -- **UT-09:** Empty-pop exception path not tested. -- **UT-10:** White-box fragile assertion style without justification. -- **UT-11:** `ON_CALL` used without `EXPECT_CALL` verification. -- **UT-12:** Test couples to internal state instead of externally observable behavior. -- **UT-13:** Mocked dependency failure path never exercised. - -## Pass Criteria -- Tool should flag missing negative tests, boundary-value gaps, and weak interaction verification in mocks. diff --git a/score/evaluation/unit_test_quality/unit_test_quality.h b/score/evaluation/unit_test_quality/unit_test_quality.h index bf3630e..ad4edae 100644 --- a/score/evaluation/unit_test_quality/unit_test_quality.h +++ b/score/evaluation/unit_test_quality/unit_test_quality.h @@ -1,22 +1,4 @@ -// ******************************************************************************* -// Copyright (c) 2025 Contributors to the Eclipse Foundation -// -// See the NOTICE file(s) distributed with this work for additional -// information regarding copyright ownership. -// -// This program and the accompanying materials are made available under the -// terms of the Apache License Version 2.0 which is available at -// https://www.apache.org/licenses/LICENSE-2.0 -// -// SPDX-License-Identifier: Apache-2.0 -// ******************************************************************************* - -/// @file unit_test_quality.h -/// @brief Production source code used as the subject under test for §11. -/// -/// The class designs here are intentionally crafted to expose missing or -/// weak test coverage that a code-review tool should flag in the -/// accompanying test file (unit_test_quality_test.cpp). + #ifndef SCORE_EVALUATION_UNIT_TEST_QUALITY_H #define SCORE_EVALUATION_UNIT_TEST_QUALITY_H @@ -31,14 +13,9 @@ namespace score namespace evaluation { -// --------------------------------------------------------------------------- -// TokenBucket – simple rate limiter used by [UT-01..UT-04]. -// --------------------------------------------------------------------------- class TokenBucket { public: - /// @param capacity Maximum number of tokens the bucket can hold. - /// @param refill_rate Tokens added per Refill() call (must be > 0). explicit TokenBucket(int capacity, int refill_rate) : capacity_(capacity), tokens_(capacity), refill_rate_(refill_rate) { @@ -48,7 +25,6 @@ class TokenBucket } } - /// Try to consume @p n tokens. Returns true on success, false if not enough tokens. bool TryConsume(int n) { if (n <= 0) return true; @@ -60,7 +36,6 @@ class TokenBucket return false; } - /// Refill the bucket; tokens are capped at capacity. void Refill() { tokens_ = std::min(tokens_ + refill_rate_, capacity_); @@ -75,19 +50,13 @@ class TokenBucket int refill_rate_; }; -// --------------------------------------------------------------------------- -// StringProcessor – used by [UT-05..UT-07]. -// --------------------------------------------------------------------------- class StringProcessor { public: - /// Trims leading and trailing ASCII whitespace. static std::string Trim(const std::string& s); - /// Splits @p s on the single-character @p delimiter. static std::vector Split(const std::string& s, char delimiter); - /// Returns @p s with every ASCII letter converted to upper-case. static std::string ToUpperCase(const std::string& s); }; @@ -116,7 +85,7 @@ inline std::vector StringProcessor::Split(const std::string& s, cha token += c; } } - result.push_back(token); // last token (may be empty) + result.push_back(token); return result; } @@ -130,9 +99,6 @@ inline std::string StringProcessor::ToUpperCase(const std::string& s) return out; } -// --------------------------------------------------------------------------- -// CircularBuffer – fixed-size FIFO used by [UT-08..UT-10]. -// --------------------------------------------------------------------------- template class CircularBuffer { @@ -143,7 +109,6 @@ class CircularBuffer if (capacity == 0) throw std::invalid_argument("capacity must be > 0"); } - /// Push an element; overwrites oldest element when full. void Push(const T& value) { buf_[tail_] = value; @@ -154,11 +119,10 @@ class CircularBuffer } else { - head_ = (head_ + 1) % capacity_; // overwrite: advance head + head_ = (head_ + 1) % capacity_; } } - /// Pop the oldest element; throws std::underflow_error when empty. T Pop() { if (size_ == 0) throw std::underflow_error("CircularBuffer is empty"); @@ -181,9 +145,6 @@ class CircularBuffer std::size_t capacity_; }; -// --------------------------------------------------------------------------- -// PaymentProcessor – used by [UT-11..UT-13] (dependency injection / mocking). -// --------------------------------------------------------------------------- class IPaymentGateway { public: @@ -197,7 +158,6 @@ class PaymentProcessor public: explicit PaymentProcessor(IPaymentGateway& gateway) : gateway_(gateway) {} - /// Returns true and records the transaction if the charge succeeds. bool ProcessPayment(const std::string& account, double amount) { if (account.empty() || amount <= 0.0) return false; @@ -209,11 +169,10 @@ class PaymentProcessor return false; } - /// Refunds the last successful charge; no-op if log is empty. bool RefundLast(const std::string& account) { if (transaction_log_.empty()) return false; - const double refund_amount = 1.0; // simplified: always refunds 1.0 + const double refund_amount = 1.0; return gateway_.Refund(account, refund_amount); } @@ -227,7 +186,7 @@ class PaymentProcessor std::vector transaction_log_; }; -} // namespace evaluation -} // namespace score +} +} #endif // SCORE_EVALUATION_UNIT_TEST_QUALITY_H diff --git a/score/evaluation/unit_test_quality/unit_test_quality_test.cpp b/score/evaluation/unit_test_quality/unit_test_quality_test.cpp index a6f9c25..2161486 100644 --- a/score/evaluation/unit_test_quality/unit_test_quality_test.cpp +++ b/score/evaluation/unit_test_quality/unit_test_quality_test.cpp @@ -1,42 +1,4 @@ -// ******************************************************************************* -// Copyright (c) 2025 Contributors to the Eclipse Foundation -// -// See the NOTICE file(s) distributed with this work for additional -// information regarding copyright ownership. -// -// This program and the accompanying materials are made available under the -// terms of the Apache License Version 2.0 which is available at -// https://www.apache.org/licenses/LICENSE-2.0 -// -// SPDX-License-Identifier: Apache-2.0 -// ******************************************************************************* -/// @file unit_test_quality_test.cpp -/// @brief Deliberately flawed GoogleTest suite for code-review tool evaluation. -/// -/// Issues planted (see EVALUATION_GUIDE.md §11 for expected review comments): -/// -/// TokenBucket tests -/// [UT-01] Test with no assertion – verifies nothing. -/// [UT-02] Single happy-path test; boundary / negative cases missing. -/// [UT-03] Hard-coded magic numbers in assertions with no explanation. -/// [UT-04] Constructor exception NOT tested. -/// -/// StringProcessor tests -/// [UT-05] Copy-paste test body – two tests are byte-for-byte identical. -/// [UT-06] Only one delimiter tested in Split(); empty / multi-token edge cases absent. -/// [UT-07] Test name is misleading / generic ("Test1", "Test2"). -/// -/// CircularBuffer tests -/// [UT-08] Overwrite-when-full behaviour not tested. -/// [UT-09] Pop() on empty buffer exception NOT tested. -/// [UT-10] Test uses implementation-internal knowledge (white-box) without comment. -/// -/// PaymentProcessor / mock tests -/// [UT-11] Mock never sets up EXPECT_CALL – mock methods never verified. -/// [UT-12] Test calls production code and asserts on mock internal state directly -/// (brittle: breaks on any internal refactor). -/// [UT-13] Failure path (gateway returns false) is never exercised. #include "score/evaluation/unit_test_quality/unit_test_quality.h" @@ -50,25 +12,12 @@ namespace evaluation namespace test { -// =========================================================================== -// TokenBucket tests -// =========================================================================== - -// --------------------------------------------------------------------------- -// [UT-01] Test body contains no assertion – the test always passes regardless -// of what TryConsume() actually returns. -// --------------------------------------------------------------------------- TEST(TokenBucketTest, ConsumeDoesNotCrash) { TokenBucket bucket(10, 2); - bucket.TryConsume(3); // [UT-01] return value discarded; no EXPECT/ASSERT + bucket.TryConsume(3); } -// --------------------------------------------------------------------------- -// [UT-02] Only the "enough tokens" path is exercised. -// Missing: consume more than available, consume exactly 0, consume -// exactly capacity, multiple consecutive consumes, refill cap at capacity. -// --------------------------------------------------------------------------- TEST(TokenBucketTest, ConsumeSucceedsWhenEnoughTokens) { TokenBucket bucket(10, 2); @@ -76,39 +25,19 @@ TEST(TokenBucketTest, ConsumeSucceedsWhenEnoughTokens) EXPECT_EQ(bucket.AvailableTokens(), 5); } -// --------------------------------------------------------------------------- -// [UT-03] Magic literals 10, 2, 7, 3 with no explanation of what they represent -// or why these specific values exercise meaningful behaviour. -// --------------------------------------------------------------------------- TEST(TokenBucketTest, RefillAddsTokens) { - TokenBucket bucket(10, 2); // [UT-03] why 10 capacity, 2 refill? - bucket.TryConsume(7); // [UT-03] why consume 7? + TokenBucket bucket(10, 2); + bucket.TryConsume(7); bucket.Refill(); - EXPECT_EQ(bucket.AvailableTokens(), 5); // [UT-03] 10-7+2=5, but not documented + EXPECT_EQ(bucket.AvailableTokens(), 5); } -// --------------------------------------------------------------------------- -// [UT-04] The constructor throws std::invalid_argument for non-positive arguments, -// but this is never tested. -// Missing: EXPECT_THROW(TokenBucket(0, 1), std::invalid_argument); -// EXPECT_THROW(TokenBucket(5, 0), std::invalid_argument); -// EXPECT_THROW(TokenBucket(-1, 1), std::invalid_argument); -// --------------------------------------------------------------------------- TEST(TokenBucketTest, ConstructWithValidArgs) { EXPECT_NO_THROW(TokenBucket(5, 1)); - // [UT-04] Only the non-throwing path tested; negative/zero args never tested. } -// =========================================================================== -// StringProcessor tests -// =========================================================================== - -// --------------------------------------------------------------------------- -// [UT-05] Two tests have identical bodies – copy-paste error; one of them -// covers nothing new and gives false confidence. -// --------------------------------------------------------------------------- TEST(StringProcessorTest, TrimRemovesLeadingSpaces) { EXPECT_EQ(StringProcessor::Trim(" hello"), "hello"); @@ -116,70 +45,39 @@ TEST(StringProcessorTest, TrimRemovesLeadingSpaces) TEST(StringProcessorTest, TrimRemovesTrailingSpaces) { - EXPECT_EQ(StringProcessor::Trim(" hello"), "hello"); // [UT-05] identical to above - // Should be: EXPECT_EQ(StringProcessor::Trim("hello "), "hello"); + EXPECT_EQ(StringProcessor::Trim(" hello"), "hello"); } -// --------------------------------------------------------------------------- -// [UT-06] Split() tested with only a single simple case. -// Missing edge cases: empty string, delimiter at start/end, consecutive -// delimiters, string with no delimiter present, single-character string. -// --------------------------------------------------------------------------- TEST(StringProcessorTest, SplitOnComma) { const auto parts = StringProcessor::Split("a,b,c", ','); EXPECT_EQ(parts.size(), 3u); - // [UT-06] Only the happy path; none of the boundary conditions checked. } -// --------------------------------------------------------------------------- -// [UT-07] Generic, non-descriptive test names give no indication of what -// behaviour is being verified or what the failure means. -// --------------------------------------------------------------------------- -TEST(StringProcessorTest, Test1) // [UT-07] what does Test1 verify? +TEST(StringProcessorTest, Test1) { EXPECT_EQ(StringProcessor::ToUpperCase("hello"), "HELLO"); } -TEST(StringProcessorTest, Test2) // [UT-07] what does Test2 verify? +TEST(StringProcessorTest, Test2) { EXPECT_EQ(StringProcessor::ToUpperCase("World"), "WORLD"); } -// =========================================================================== -// CircularBuffer tests -// =========================================================================== - -// --------------------------------------------------------------------------- -// [UT-08] Full-capacity overwrite behaviour is never tested. -// A CircularBuffer should silently overwrite the oldest element when -// Push() is called on a full buffer. This contract is untested. -// --------------------------------------------------------------------------- TEST(CircularBufferTest, PushAndPopSingleElement) { CircularBuffer cb(3); cb.Push(42); EXPECT_EQ(cb.Pop(), 42); - // [UT-08] Never fills the buffer to capacity, never verifies overwrite. } -// --------------------------------------------------------------------------- -// [UT-09] Pop() on an empty buffer throws std::underflow_error – never tested. -// Missing: EXPECT_THROW(cb.Pop(), std::underflow_error); -// --------------------------------------------------------------------------- TEST(CircularBufferTest, IsEmptyAfterConstruction) { CircularBuffer cb(4); EXPECT_TRUE(cb.IsEmpty()); EXPECT_EQ(cb.Size(), 0u); - // [UT-09] Destructive empty-pop exception path never exercised. } -// --------------------------------------------------------------------------- -// [UT-10] The test inspects head_/tail_ indirectly via Size() in a way that -// assumes the internal layout of the circular buffer (white-box testing -// without any comment explaining why white-box is justified here). -// --------------------------------------------------------------------------- TEST(CircularBufferTest, SizeTracksCorrectly) { CircularBuffer cb(3); @@ -187,20 +85,12 @@ TEST(CircularBufferTest, SizeTracksCorrectly) cb.Push(2); EXPECT_EQ(cb.Size(), 2u); cb.Pop(); - // [UT-10] Using Size() to infer internal cursor positions without comment. EXPECT_EQ(cb.Size(), 1u); cb.Push(3); cb.Push(4); - EXPECT_EQ(cb.Size(), 3u); // [UT-10] relies on exact wrap-around behaviour + EXPECT_EQ(cb.Size(), 3u); } -// =========================================================================== -// PaymentProcessor / Mock tests -// =========================================================================== - -// --------------------------------------------------------------------------- -// Mock gateway for PaymentProcessor. -// --------------------------------------------------------------------------- class MockPaymentGateway : public IPaymentGateway { public: @@ -208,15 +98,9 @@ class MockPaymentGateway : public IPaymentGateway MOCK_METHOD(bool, Refund, (const std::string& account, double amount), (override)); }; -// --------------------------------------------------------------------------- -// [UT-11] EXPECT_CALL is never set up, so the mock framework cannot verify -// that Charge() was actually called with the expected arguments. -// The test passes even if ProcessPayment() never calls gateway_.Charge(). -// --------------------------------------------------------------------------- TEST(PaymentProcessorTest, ProcessPaymentCallsGateway) { MockPaymentGateway mock_gateway; - // [UT-11] No EXPECT_CALL(...) – the mock records nothing and verifies nothing. ON_CALL(mock_gateway, Charge(::testing::_, ::testing::_)) .WillByDefault(::testing::Return(true)); @@ -224,12 +108,6 @@ TEST(PaymentProcessorTest, ProcessPaymentCallsGateway) EXPECT_TRUE(processor.ProcessPayment("ACC-001", 50.0)); } -// --------------------------------------------------------------------------- -// [UT-12] The test asserts on transaction_log_.size() == 1 via TransactionLog(), -// which is an implementation detail. If the log is ever refactored -// (e.g., entries stored differently), the test breaks for the wrong reason. -// The relevant contract is "payment succeeds", not "internal log has size 1". -// --------------------------------------------------------------------------- TEST(PaymentProcessorTest, TransactionLogUpdatedAfterCharge) { MockPaymentGateway mock_gateway; @@ -238,26 +116,18 @@ TEST(PaymentProcessorTest, TransactionLogUpdatedAfterCharge) PaymentProcessor processor(mock_gateway); processor.ProcessPayment("ACC-002", 25.0); - // [UT-12] Testing internal state (log size) rather than observable behaviour. EXPECT_EQ(processor.TransactionLog().size(), 1u); } -// --------------------------------------------------------------------------- -// [UT-13] The gateway's failure path (Charge returns false) is never tested. -// Missing: a test where Charge() returns false and ProcessPayment() -// must return false and must NOT append to the transaction log. -// --------------------------------------------------------------------------- TEST(PaymentProcessorTest, ProcessPaymentWithInvalidAccount) { MockPaymentGateway mock_gateway; PaymentProcessor processor(mock_gateway); - // [UT-13] Tests only empty-account guard; gateway-failure path absent. EXPECT_FALSE(processor.ProcessPayment("", 10.0)); EXPECT_FALSE(processor.ProcessPayment("ACC-003", 0.0)); - // Missing: mock returns false → ProcessPayment should return false. } -} // namespace test -} // namespace evaluation -} // namespace score +} +} +}