Fix #14334: Support more msbuild conditional constructs #8039

autoantwort · 2025-12-16T18:33:09Z

No description provided.

lib/importproject.cpp

firewave · 2025-12-16T20:38:01Z

Thanks for your contribution.

Please avoid adding test-only code in the production code. See e04019c for an approach. If you inherit from ProjectConfiguration the added function could be moved into the test code and the additional constructor and moved implementation could me made protected.

firewave · 2025-12-16T20:38:50Z

lib/importproject.cpp

+            catch (const std::runtime_error& r)
+			{
+				return false;
+			}


We should report the exception as a debug or verbose message.

Should this use the ErrorLogger infrastructure?

Yes. reportErr() for a debug message or reportOut() for a verbose message. Not sure which would be more appropriate.

firewave · 2025-12-16T20:43:44Z

lib/importproject.cpp

-            tokenlist.createAst();
-            for (const Token *tok = tokenlist.front(); tok; tok = tok->next()) {
-                if (tok->str() == "(" && tok->astOperand1() && tok->astOperand2()) {
-                    // TODO: this is wrong - it is Contains() not Equals()


We should have filed a ticket for this since fixing it is a behavior change. Are you able to do that or should I file one?

Could you do that? :)

Will do later. I want to have a reproducer first.

autoantwort · 2025-12-17T12:43:30Z

Please avoid adding test-only code in the production code. See e04019c for an approach. If you inherit from ProjectConfiguration the added function could be moved into the test code and the additional constructor and moved implementation could me made protected.

Currently ProjectConfiguration and ConditionalGroup are an implementation detail and defined in the cpp file. I would have to move the classes to the header. Then they become an public interface only because of test-only code.
My thought was that exposing a single test-only function is better than exposing two complete implementation details classes.

firewave · 2025-12-17T12:47:37Z

Currently ProjectConfiguration and ConditionalGroup are an implementation detail and defined in the cpp file.

Sorry that I missed that.

I would have to move the classes to the header. Then they become an public interface only because of test-only code.
My thought was that exposing a single test-only function is better than exposing two complete implementation details classes.

Let's get the CI to pass first and the other stuff addressed and then I will have another look. It won't be merged before we made the 2.19 release anyways.

danmar · 2026-01-02T11:34:23Z

lib/importproject.cpp


+        static bool evalExpression(std::string expr /* should be a std::string_view */, const ProjectConfiguration& p)
+        {
+            const auto startsHere = [&](size_t& i, std::string string)


I have the feeling this could be simplified using string::compare

Ok will it change to

if (expr.compare(i, string.length(), string) == 0) { i += string.length(); return true; } return false;

danmar · 2026-01-02T11:35:15Z

lib/importproject.cpp

+                }
+                return false;
+            };
+            const auto skipSpaces = [&](size_t& i)


I think i = expr.find_first_not_of(' ',i); can be used instead.

danmar · 2026-01-02T11:37:16Z

lib/importproject.cpp

+                    throw std::runtime_error("Expecting a start of a string!");
+                }
+                auto start = ++i;
+                while (i < expr.length() && expr[i] != '\'') ++i;


can there be some escape sequences we should handle i.e. if expr is 'a\'b' ?

No it seems that is not possible

danmar · 2026-01-02T11:39:53Z

lib/importproject.cpp

+                if (startsHere(i, "$(Configuration."))
+                {
+                    initialized = true;
+                    if (startsHere(i, "Contains("))


is it possible to have spaces.. Contains ( ?

Never have seen this, but it seems to be possible.

danmar · 2026-01-02T11:45:47Z

lib/importproject.cpp

+                {
+                    return !evalExpression(expr.substr(i + 1), p);
+                }
+                if (startsHere(i, "And"))


this will be true if expr contains AndXY.

Hm that is true. Otherwise this is then also an invalid msbuild project that does not build...

lib/importproject.cpp

            }
        }

+        static bool evalExpression(std::string expr /* should be a std::string_view */, const ProjectConfiguration& p)


sonarqubecloud · 2026-01-02T15:30:39Z

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

github-advanced-security bot found potential problems Dec 16, 2025

View reviewed changes

lib/importproject.cpp Fixed Show fixed Hide fixed

autoantwort force-pushed the fix-14334 branch from a8ea143 to dade829 Compare December 16, 2025 18:43

firewave reviewed Dec 16, 2025

View reviewed changes

firewave marked this pull request as draft December 16, 2025 20:44

autoantwort force-pushed the fix-14334 branch 7 times, most recently from a327364 to e674fb6 Compare December 17, 2025 17:57

autoantwort requested a review from firewave December 30, 2025 22:37

autoantwort marked this pull request as ready for review December 30, 2025 22:37

autoantwort force-pushed the fix-14334 branch 3 times, most recently from 0818e64 to f0810d5 Compare December 31, 2025 02:27

danmar reviewed Jan 2, 2026

View reviewed changes

autoantwort force-pushed the fix-14334 branch from f0810d5 to a16e00d Compare January 2, 2026 14:50

github-advanced-security bot found potential problems Jan 2, 2026

View reviewed changes

Fix #14334: Support more msbuild conditional constructs

21c60df

autoantwort force-pushed the fix-14334 branch from a16e00d to 21c60df Compare January 2, 2026 15:28

Fix #14334: Support more msbuild conditional constructs #8039

Are you sure you want to change the base?

Fix #14334: Support more msbuild conditional constructs #8039

Conversation

autoantwort commented Dec 16, 2025

Uh oh!

Uh oh!

firewave commented Dec 16, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

autoantwort commented Dec 17, 2025

Uh oh!

firewave commented Dec 17, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

danmar Jan 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Check warning

sonarqubecloud bot commented Jan 2, 2026

Quality Gate passed

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

danmar Jan 2, 2026 •

edited

Loading