From ab8c25123963998833635a4b6c4f072009473fda Mon Sep 17 00:00:00 2001 From: Patil Date: Mon, 23 Mar 2026 21:10:24 +0530 Subject: [PATCH] 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