Developer forums (C::B DEVELOPMENT STRICTLY!) > Development

Code dublication on compilergcc.cpp

(1/2) > >>

BlueHazzard:
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);
    }

--- End code ---

if we look at the declaration of GetMacrosManager()::ReplaceEnvVars (macrosmanager.h:32)

--- Code: ---void ReplaceEnvVars(wxString& buffer) { ReplaceMacros(buffer); }  /* misnomer, should be ReplaceVariables */;
--- End code ---

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

any thoughts?

oBFusCATed:
Are they doing the same thing?
So if you remove the second call are you going to preserve current behaviour?

BlueHazzard:

--- Quote from: oBFusCATed on May 04, 2021, 01:13:00 am ---So if you remove the second call are you going to preserve current behaviour?

--- End quote ---
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);

--- End code ---
this will translate to

--- Code: --- Manager::Get()->GetMacrosManager()->ReplaceMacros(command, target);  // (1)
Manager::Get()->GetMacrosManager()->ReplaceMacros(command, nullptr);  // (2)

--- End code ---
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();

--- End code ---

BlueHazzard:
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)

--- End code ---
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/

oBFusCATed:
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. :(

Navigation

[0] Message Index

[#] Next page