Author Topic: Code dublication on compilergcc.cpp  (Read 35367 times)

Offline BlueHazzard

  • Developer
  • Lives here!
  • *****
  • Posts: 3353
Code dublication on compilergcc.cpp
« on: May 03, 2021, 11:52:39 pm »
Hi,
i tried to fix https://sourceforge.net/p/codeblocks/tickets/328/ (the fix is already in the code below)
and found some, what i think is, code duplication sdk:
Code
    if (   target->GetTargetType() == ttDynamicLib
        || target->GetTargetType() == ttStaticLib )
    {
        // check for hostapp
        if (target->GetHostApplication().IsEmpty())
        {
            cbMessageBox(_("You must select a host application to \"run\" a library..."));
            m_pProject->SetCurrentlyCompilingTarget(0);
            return -1;
        }

        command << hostapStr << strSPACE;
        command << target->GetExecutionParameters();
        Manager::Get()->GetMacrosManager()->ReplaceMacros(command);
    }
    else if (target->GetTargetType() != ttCommandsOnly)
    {
        command << execStr << strSPACE;
        command << target->GetExecutionParameters();
        // each shell execution must be enclosed to "":
        // xterm -T X -e /bin/sh -c "/usr/bin/cb_console_runner X"
        if (commandIsQuoted)
            command << strQUOTE;
        Manager::Get()->GetMacrosManager()->ReplaceMacros(command, target);
        Manager::Get()->GetMacrosManager()->ReplaceEnvVars(command);
    }
    else
    {
        // commands-only target?
        if (target->GetHostApplication().IsEmpty())
        {
            cbMessageBox(_("You must select a host application to \"run\" a commands-only target..."));
            m_pProject->SetCurrentlyCompilingTarget(0);
            return -1;
        }
        command << hostapStr << strSPACE;
        command << target->GetExecutionParameters();
        Manager::Get()->GetMacrosManager()->ReplaceMacros(command, target);
        Manager::Get()->GetMacrosManager()->ReplaceEnvVars(command);
    }

if we look at the declaration of GetMacrosManager()::ReplaceEnvVars (macrosmanager.h:32)
Code
void ReplaceEnvVars(wxString& buffer) { ReplaceMacros(buffer); }  /* misnomer, should be ReplaceVariables */;

So this is only a ReplaceMacros. I think we can remove all this calls in the above functions

any thoughts?

Offline oBFusCATed

  • Developer
  • Lives here!
  • *****
  • Posts: 13413
    • Travis build status
Re: Code dublication on compilergcc.cpp
« Reply #1 on: May 04, 2021, 01:13:00 am »
Are they doing the same thing?
So if you remove the second call are you going to preserve current behaviour?
(most of the time I ignore long posts)
[strangers don't send me private messages, I'll ignore them; post a topic in the forum, but first read the rules!]

Offline BlueHazzard

  • Developer
  • Lives here!
  • *****
  • Posts: 3353
Re: Code dublication on compilergcc.cpp
« Reply #2 on: May 04, 2021, 10:27:19 am »
So if you remove the second call are you going to preserve current behaviour?
I am not 100% sure, i will test, but i am not 100% sure how...

I think that the current behavior is broken:
if we look at this two lines of code:
Code
Manager::Get()->GetMacrosManager()->ReplaceMacros(command, target);
Manager::Get()->GetMacrosManager()->ReplaceEnvVars(command);
this will translate to
Code
 Manager::Get()->GetMacrosManager()->ReplaceMacros(command, target);  // (1)
Manager::Get()->GetMacrosManager()->ReplaceMacros(command, nullptr);  // (2)
what will replace all variables related to the current build target (1) and then with the current active target (2)[1]. If the current build target is different from the active target this is wrong behavior. I currently do not know if this can happen (i have in my mind a bug report that reported this error, but i can not find it)

[1]
in MacrosManager::ReplaceMacros() (macrosmanager.cpp:538):
Code
 const cbProject* project = target
                             ? target->GetParentProject()
                             : Manager::Get()->GetProjectManager()->GetActiveProject();

Offline BlueHazzard

  • Developer
  • Lives here!
  • *****
  • Posts: 3353
Re: Code dublication on compilergcc.cpp
« Reply #3 on: May 05, 2021, 12:26:57 am »
Ok, i tested it (somehow*) extensively
Code
diff --git a/src/plugins/compilergcc/compilergcc.cpp b/src/plugins/compilergcc/compilergcc.cpp
index 28f3dd1c0..30540db5c 100644
--- a/src/plugins/compilergcc/compilergcc.cpp
+++ b/src/plugins/compilergcc/compilergcc.cpp
@@ -2015,7 +2015,7 @@ int CompilerGCC::Run(ProjectBuildTarget* target)
             m_pProject->SetCurrentlyCompilingTarget(0);
             return -1;
         }
-
+        Manager::Get()->GetMacrosManager()->ReplaceEnvVars(hostapStr);
         command << hostapStr << strSPACE;
         command << target->GetExecutionParameters();
     }
@@ -2028,6 +2028,8 @@ int CompilerGCC::Run(ProjectBuildTarget* target)
         // here is last \"
         if (commandIsQuoted)
             command << strQUOTE;
+        Manager::Get()->GetMacrosManager()->ReplaceMacros(command, target);
+        Manager::Get()->GetMacrosManager()->ReplaceEnvVars(command);
     }
     else
     {
@@ -2040,8 +2042,10 @@ int CompilerGCC::Run(ProjectBuildTarget* target)
         }
         command << hostapStr << strSPACE;
         command << target->GetExecutionParameters();
+        Manager::Get()->GetMacrosManager()->ReplaceMacros(command, target);
+        Manager::Get()->GetMacrosManager()->ReplaceEnvVars(command);
     }
-    Manager::Get()->GetMacrosManager()->ReplaceMacros(command, target);
+
     wxString script = command;

     if (platform::macosx)
this patch removes code/ execution duplication and fixes the ticket by enabling macro expansion for shared libraries

i could not trigger any bug i had in mind, i also performed a forum search, but could not find any report about this...

if there are no objections i would apply this patch in the next days

* it is quite difficult to think about any possibility this complex code is called. I have tried all normal ways i could think of to trigger the related code portions. To find out if this breaks some obscure workflow we probably have to wait for complains after the patch is applied... obligatory xkcd: https://xkcd.com/1172/

Offline oBFusCATed

  • Developer
  • Lives here!
  • *****
  • Posts: 13413
    • Travis build status
Re: Code dublication on compilergcc.cpp
« Reply #4 on: May 05, 2021, 10:23:57 pm »
I'm fine as long as C::B compiles. :)

But there are still two calls. Are they really needed? Can you add a comment why they are needed?
Please don't repeat Manager::Get()->GetMacrosManager() on every line. If you need it more than once use a variable. (I know the original code was written in this sloppy way and it is not your fault, but you can make the code better without too much effort)

This is what happens when there is no regression testing - forward progress is hard to define. :(
(most of the time I ignore long posts)
[strangers don't send me private messages, I'll ignore them; post a topic in the forum, but first read the rules!]

Offline BlueHazzard

  • Developer
  • Lives here!
  • *****
  • Posts: 3353
Re: Code dublication on compilergcc.cpp
« Reply #5 on: May 05, 2021, 10:42:00 pm »
Quote
Please don't repeat Manager::Get()->GetMacrosManager() on every line. If you need it more than once use a variable. (I know the original code was written in this sloppy way and it is not your fault, but you can make the code better without too much effort)

Ahhhh sh****, you have to reverse the patch... So where a + should be a - 
So i remove the double calls, not add them...
Somehow i pasted the wrong patch....