From 9c9b4871433498f34ed1b868aea1418310ad0aeb Mon Sep 17 00:00:00 2001
From: Udi Finkelstein <github@udifink.com>
Date: Tue, 2 Apr 2024 22:21:20 +0300
Subject: [PATCH] PR: Make command-line defines stronger than `define's inside
 Verilog files (#929)

---
 include/slang/parsing/Preprocessor.h          |  1 +
 source/parsing/Preprocessor.cpp               |  8 +++--
 tests/unittests/parsing/PreprocessorTests.cpp | 35 +++++++++++++++++++
 3 files changed, 42 insertions(+), 2 deletions(-)

diff --git a/include/slang/parsing/Preprocessor.h b/include/slang/parsing/Preprocessor.h
index c4e1445c7..2db9eb1b9 100644
--- a/include/slang/parsing/Preprocessor.h
+++ b/include/slang/parsing/Preprocessor.h
@@ -250,6 +250,7 @@ class SLANG_EXPORT Preprocessor {
         const syntax::DefineDirectiveSyntax* syntax = nullptr;
         MacroIntrinsic intrinsic = MacroIntrinsic::None;
         bool builtIn = false;
+        bool commandLine = false;
 
         MacroDef() = default;
         MacroDef(const syntax::DefineDirectiveSyntax* syntax) : syntax(syntax) {}
diff --git a/source/parsing/Preprocessor.cpp b/source/parsing/Preprocessor.cpp
index 194cbaa62..936b14fe4 100644
--- a/source/parsing/Preprocessor.cpp
+++ b/source/parsing/Preprocessor.cpp
@@ -119,9 +119,11 @@ void Preprocessor::predefine(const std::string& definition, std::string_view nam
     // Look for the macro in the temporary preprocessor's macro map.
     // Any macros found that are not the built-in intrinsic macros should
     // be copied over to our own map.
-    for (const auto& pair : pp.macros) {
-        if (!pair.second.isIntrinsic())
+    for (auto& pair : pp.macros) {
+        if (!pair.second.isIntrinsic()) {
+            pair.second.commandLine = true;
             macros.insert(pair);
+        }
     }
 }
 
@@ -640,6 +642,8 @@ Trivia Preprocessor::handleDefineDirective(Token directive) {
             addDiag(diag::InvalidMacroName, name.range());
             bad = true;
         }
+        else if (it->second.commandLine)
+            bad = true; // not really bad, but commandLine args has precedence so we skip this
         else if (!bad && it->second.valid() && !isSameMacro(*result, *it->second.syntax)) {
             auto& diag = addDiag(diag::RedefiningMacro, name.range());
             diag << name.valueText();
diff --git a/tests/unittests/parsing/PreprocessorTests.cpp b/tests/unittests/parsing/PreprocessorTests.cpp
index 4f0d87b58..345421a7c 100644
--- a/tests/unittests/parsing/PreprocessorTests.cpp
+++ b/tests/unittests/parsing/PreprocessorTests.cpp
@@ -1382,6 +1382,41 @@ TEST_CASE("Preprocessor API") {
     CHECK(pp.getDefinedMacros().size() == 19);
 }
 
+TEST_CASE("Command-line defines priority over `define") {
+    PreprocessorOptions ppOptions;
+    ppOptions.predefines.emplace_back("A=2");
+    ppOptions.predefines.emplace_back("B=2");
+    ppOptions.predefines.emplace_back("C=2");
+
+    Bag options;
+    options.set(ppOptions);
+
+    auto& text = R"(
+`define A 1
+`undef B
+`undef C
+`define C 1
+)";
+    Preprocessor preprocessor(getSourceManager(), alloc, diagnostics, options);
+    preprocessor.pushSource(text);
+
+    while (true) {
+        Token token = preprocessor.next();
+        if (token.kind == TokenKind::EndOfFile)
+            break;
+    }
+
+    CHECK(!preprocessor.isDefined("B"));
+    CHECK(preprocessor.isDefined("A"));
+    CHECK(preprocessor.isDefined("C"));
+    for (auto macro : preprocessor.getDefinedMacros()) {
+        if (macro->name.toString() == "A")
+            CHECK(macro->body[0].toString() == "2");
+        if (macro->name.toString() == "C")
+            CHECK(macro->body[0].toString() == "1");
+    }
+}
+
 TEST_CASE("Undef builtin") {
     auto& text = R"(
 `undef __slang__