From: Keith Marshall Date: Tue, 23 Jun 2020 17:01:47 +0000 (+0100) Subject: Eliminate invalid comparisons of "this" with nullptr. X-Git-Url: http://git.osdn.net/view?p=mingw%2Fmingw-get.git;a=commitdiff_plain;h=1c523d48fbc447f6c81b46afcf962dabb5816f74;ds=sidebyside Eliminate invalid comparisons of "this" with nullptr. --- diff --git a/ChangeLog b/ChangeLog index f52a34d..6264207 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,3 +1,73 @@ +2020-06-23 Keith Marshall + + Eliminate invalid comparisons of "this" with nullptr. + + C++ forbids calling any non-static class member function through + a null pointer, but makes it impossible to verify, within any such + function; the result of comparing the "this" pointer with nullptr + is deemed to be undefined behaviour. + + * src/climain.cpp (pkgActionItem::GetScheduledSourceArchives) + [this != NULL]: Do not test; execute dependent code unconditionally. + + * src/dllhook.cpp (pkgXmlNodeStack::pop) [this == NULL]: Remove + invalid comparison; condition should never arise, at point of call. + (pkgSetupAction::UpdateDatabase) [this != NULL]: Relocate test... + (update_database) [setup == NULL]: ...to here; do not update. + + * src/pkgbase.h (pkgXmlNode::GetName, pkgXmlNode::GetParent) + (pkgXmlNode::GetChildren, pkgXmlNode::GetNext, pkgXmlNode::GetPropVal) + (pkgXmlNode::GetDocumentRoot, pkgXmlNode::IsElementOfType) + (pkgXmlNode::AddChild, pkgXmlNode::DeleteChild): Do not implement... + [this ? result : fallback]: ...any such checks; the behaviour will be + undefined, and the "fallback" outcome can never be achieved; simply + return the "result" outcome unconditionally. + (pkgActionItem::HasAttribute, pkgActionItem::SelectPackage) + [this != NULL]: Cannot verify this; return result unconditionally. + (pkgActionItem::Selection) [this == NULL]: Test is invalid; remove it. + (pkgActionItem::CancelScheduledAction): Change return type to void. + (pkgXmlDocument::ExecuteActions) [actions == NULL]: Do not execute. + + * src/pkgdata.cpp (pkgActionItem::EnumeratePendingActions) + [this != NULL]: Cannot verify; relocate test... + (AppWindowMaker::UpdatePackageMenuBindings): ...to here; verify... + [pkgData->Schedule() != NULL]: ...this, before attempting to invoke... + (pkgData->Schedule()->EnumeratePendingActions): ...this. + (AppWindowMaker::UnmarkSelectedPackage): Likewise, verify both... + [pkgData->Schedule() != NULL]: ...this, and then... + [pkgData->Schedule()->GetReference() != NULL]: ...this, before... + (pkgData->Schedule()->GetReference()->CancelScheduledAction): ...this. + (pkgActionItem::CancelScheduledAction) [this != NULL]: Remove test; it + results in undefined behaviour, and in any case, has become redundant. + Change return type to void; set flags, but otherwise return nothing. + + * src/pkgdeps.cpp (pkgActionItem::GetReference) [this != NULL]: Remove + invalid test; it has been made redundant, by testing at point of call. + (pkgXmlDocument::Schedule) [this != NULL]: Likewise; additionally... + [component != NULL]: ...verify this, before attempting to test... + [component->FindNextAssociate() != NULL]: ...this. + + * src/pkgexec.cpp (pkgActionItem::Execute) [this != NULL] + (pkgActionItem::Append, pkgActionItem::Insert) [this == NULL]: Remove + invalid tests; delegate onus for validation to respective call sites. + (pkgXmlDocument::Schedule): Validate referring pointers as required. + + * src/guiexec.cpp (AppWindowMaker::LoadPackageData): Confirm that... + [pkgXmlDocument() != NULL]: ...is true, before evaluation of... + [pkgXmlDocument()->IsOK()]: ...this status check. + (AppWindowMaker::ConfirmActionRequest): Likewise, confirm that... + [pkgData->Schedule() != NULL]: ...is true, before evaluation of... + [pkgData->Schedule()->EnumeratePendingActions() > 0]: ...this. + + * src/pkgunst.cpp (pkgManifest::GetSysRootReference) [this != NULL] + * src/pkginst.cpp (pkgManifest::AddEntry) [this != NULL]: Remove test; + assume that its outcome is always effectively true, requiring callers + to guarantee that this is so. + + * src/setup.cpp (pkgSetupAction::HasAttribute) [this != NULL]: Remove + invalid test, assuming true; relocate inline implementation... + * src/setup.h: ...to here. + 2020-06-22 Keith Marshall Do not dereference nullptr in package directory trees. diff --git a/src/climain.cpp b/src/climain.cpp index f87b9d4..9230526 100644 --- a/src/climain.cpp +++ b/src/climain.cpp @@ -403,27 +403,21 @@ void pkgActionItem::GetScheduledSourceArchives( unsigned long category ) * for the case when the "--all-related" option is in effect for a * "source" or "licence" request. */ - if( this != NULL ) + pkgActionItem *scheduled = this; + while( scheduled->prev != NULL ) scheduled = scheduled->prev; + + /* For each scheduled list entry... + */ + while( scheduled != NULL ) { - /* The package list is NOT empty; ensure that we begin with - * a reference to its first entry. + /* ...process the "source" or "licence" request, as appropriate, + * in respect of the associated package... */ - pkgActionItem *scheduled = this; - while( scheduled->prev != NULL ) scheduled = scheduled->prev; - - /* For each scheduled list entry... + scheduled->GetSourceArchive( scheduled->Selection(), category ); + /* + * ...then move on to the next entry, if any. */ - while( scheduled != NULL ) - { - /* ...process the "source" or "licence" request, as appropriate, - * in respect of the associated package... - */ - scheduled->GetSourceArchive( scheduled->Selection(), category ); - /* - * ...then move on to the next entry, if any. - */ - scheduled = scheduled->next; - } + scheduled = scheduled->next; } } diff --git a/src/dllhook.cpp b/src/dllhook.cpp index eed0a62..0b851ed 100644 --- a/src/dllhook.cpp +++ b/src/dllhook.cpp @@ -172,10 +172,11 @@ void update_database( pkgSetupAction *setup ) /* Having successfully loaded the restricted profile, load the * map of the installation it specifies, and update it to reflect * the installation of the mingw-get packages we are currently - * installing. + * installing; (note that we must take care not to attempt to + * update the database, if passed a NULL setup object). */ dbase.LoadSystemMap(); - setup->UpdateDatabase( dbase ); + if( setup != NULL ) setup->UpdateDatabase( dbase ); } /* We're finished with the setup.xml profile; delete it, and free * the memory which xmlfile() allocated to store its path name. @@ -351,16 +352,12 @@ inline pkgXmlNodeStack *pkgXmlNodeStack::pop( pkgXmlNode **node ) /* Method to pop a pkgXmlNode from the top of the stack, while * saving a reference pointer for it, and returning the updated * stack pointer. - */ - if( this == NULL ) - /* - * The stack is empty; there's nothing more to do! - */ - return NULL; - - /* When the stack is NOT empty, capture the reference pointer - * for the SECOND entry (if any), store the top entry, delete - * its stack frame, and return the new stack pointer. + * + * This should never be called when the stack is empty; (this + * must be checked at the call site). When the stack is known + * to NOT be empty, capture the reference pointer for the SECOND + * entry (if any), store the top entry, delete its stack frame, + * and return the new stack pointer. */ pkgXmlNodeStack *rtn = next; *node = entry; delete this; return rtn; @@ -371,169 +368,168 @@ void pkgSetupAction::UpdateDatabase( pkgXmlDocument &dbase ) /* Method to ensure that the mingw-get package components which are * specified in the setup actions list are recorded as "installed", in * the installation database manifests. + * + * This method should NEVER be invoked on an empty setup actions list; + * (this must be verified at the call site). When it is known that the + * list is NOT empty, we must ensure that we commence processing from + * its first entry... */ - if( this != NULL ) + pkgSetupAction *current = this; + while( current->prev != NULL ) current = current->prev; + dmh_notify( DMH_INFO, "%s: updating installation database\n", setup_key ); + while( current != NULL ) { - /* The setup actions list is not empty; ensure that we commence - * processing from its first entry... + /* ...then processing all entries sequentially, in turn, + * parse the package tarname specified in the current action + * entry, to identify the associated package name, component + * class and subsystem name. */ - pkgSetupAction *current = this; - while( current->prev != NULL ) current = current->prev; - dmh_notify( DMH_INFO, "%s: updating installation database\n", setup_key ); - while( current != NULL ) - { - /* ...then processing all entries sequentially, in turn, - * parse the package tarname specified in the current action - * entry, to identify the associated package name, component - * class and subsystem name. + const char *name_fmt = "%s-%s"; + pkgSpecs lookup( current->package_name ); + char lookup_name[1 + strlen( current->package_name )]; + const char *component = lookup.GetComponentClass(); + const char *subsystem = lookup.GetSubSystemName(); + if( (component != NULL) && *component ) + /* + * The package name is qualified by an explicit component + * name; form the composite package name string. */ - const char *name_fmt = "%s-%s"; - pkgSpecs lookup( current->package_name ); - char lookup_name[1 + strlen( current->package_name )]; - const char *component = lookup.GetComponentClass(); - const char *subsystem = lookup.GetSubSystemName(); - if( (component != NULL) && *component ) - /* - * The package name is qualified by an explicit component - * name; form the composite package name string. - */ - sprintf( lookup_name, name_fmt, lookup.GetPackageName(), component ); - else - /* There is no explicit component name; just save a copy - * of the unqualified package name. - */ - strcpy( lookup_name, lookup.GetPackageName() ); + sprintf( lookup_name, name_fmt, lookup.GetPackageName(), component ); + else + /* There is no explicit component name; just save a copy + * of the unqualified package name. + */ + strcpy( lookup_name, lookup.GetPackageName() ); - /* Locate the corresponding component package entry, if any, - * in the package catalogue. + /* Locate the corresponding component package entry, if any, + * in the package catalogue. + */ + if( dbase.FindPackageByName( lookup_name, subsystem ) != NULL ) + { + /* Lookup was successful; now search the installation records, + * if any, for any matching package entry. */ - if( dbase.FindPackageByName( lookup_name, subsystem ) != NULL ) + pkgXmlNodeStack *stack = NULL; + pkgXmlNode *sysroot = dbase.GetRoot()->GetSysRoot( subsystem ); + pkgXmlNode *installed = sysroot->FindFirstAssociate( installed_key ); + while( installed != NULL ) { - /* Lookup was successful; now search the installation records, - * if any, for any matching package entry. + /* There is at least one installation record; walk the chain + * of all such records... */ - pkgXmlNodeStack *stack = NULL; - pkgXmlNode *sysroot = dbase.GetRoot()->GetSysRoot( subsystem ); - pkgXmlNode *installed = sysroot->FindFirstAssociate( installed_key ); - while( installed != NULL ) + const char *tarname = installed->GetPropVal( tarname_key, NULL ); + if( tarname != NULL ) { - /* There is at least one installation record; walk the chain - * of all such records... + /* ...extracting package and component names from the tarname + * specification within each... */ - const char *tarname = installed->GetPropVal( tarname_key, NULL ); - if( tarname != NULL ) - { - /* ...extracting package and component names from the tarname - * specification within each... + pkgSpecs ref( tarname ); + char ref_name[1 + strlen( tarname )]; + if( ((component = ref.GetComponentClass()) != NULL) && *component ) + /* + * ...once again forming the composite name, when applicable... */ - pkgSpecs ref( tarname ); - char ref_name[1 + strlen( tarname )]; - if( ((component = ref.GetComponentClass()) != NULL) && *component ) - /* - * ...once again forming the composite name, when applicable... - */ - sprintf( ref_name, name_fmt, ref.GetPackageName(), component ); - else - /* ...or simply storing the unqualified package name if not. - */ - strcpy( ref_name, ref.GetPackageName() ); - - /* Check for a match between the installed package name, and - * the name we wish to record as newly installed... + sprintf( ref_name, name_fmt, ref.GetPackageName(), component ); + else + /* ...or simply storing the unqualified package name if not. */ - if( (strcasecmp( ref_name, lookup_name ) == 0) - && (strcasecmp( tarname, current->package_name ) != 0) ) - /* - * ...pushing the current installation record on to the - * update stack, in case of a match... - */ - stack = stack->push( installed ); - } - /* ...then move on to the next installation record, if any. + strcpy( ref_name, ref.GetPackageName() ); + + /* Check for a match between the installed package name, and + * the name we wish to record as newly installed... */ - installed = installed->FindNextAssociate( installed_key ); + if( (strcasecmp( ref_name, lookup_name ) == 0) + && (strcasecmp( tarname, current->package_name ) != 0) ) + /* + * ...pushing the current installation record on to the + * update stack, in case of a match... + */ + stack = stack->push( installed ); } + /* ...then move on to the next installation record, if any. + */ + installed = installed->FindNextAssociate( installed_key ); + } - /* Create a temporary package "release" descriptor... + /* Create a temporary package "release" descriptor... + */ + pkgXmlNode *reference_hook = new pkgXmlNode( release_key ); + if( reference_hook != NULL ) + { + /* ...which we may conveniently attach to the root + * of the XML catalogue tree. */ - pkgXmlNode *reference_hook = new pkgXmlNode( release_key ); - if( reference_hook != NULL ) - { - /* ...which we may conveniently attach to the root - * of the XML catalogue tree. - */ - dbase.GetRoot()->AddChild( reference_hook ); - reference_hook->SetAttribute( tarname_key, current->package_name ); + dbase.GetRoot()->AddChild( reference_hook ); + reference_hook->SetAttribute( tarname_key, current->package_name ); - /* Run the installer... + /* Run the installer... + */ + pkgTarArchiveInstaller registration_server( reference_hook ); + if( registration_server.IsOk() ) + { + /* ...reporting the installation as a "registration" of + * the specified package, but... */ - pkgTarArchiveInstaller registration_server( reference_hook ); - if( registration_server.IsOk() ) - { - /* ...reporting the installation as a "registration" of - * the specified package, but... - */ - dmh_notify( DMH_INFO, "%s: register %s\n", - setup_key, current->package_name - ); - /* ...noting that the package content has already been - * "installed" by the setup tool, but without recording - * any details, we run this without physically extracting - * any files, to capture the side effect of compiling an - * installation record. - */ - registration_server.SaveExtractedFiles( false ); - registration_server.Process(); - } - /* With the installation record safely compiled, we may - * discard the temporary "release" descriptor from which - * we compiled it. + dmh_notify( DMH_INFO, "%s: register %s\n", + setup_key, current->package_name + ); + /* ...noting that the package content has already been + * "installed" by the setup tool, but without recording + * any details, we run this without physically extracting + * any files, to capture the side effect of compiling an + * installation record. */ - dbase.GetRoot()->DeleteChild( reference_hook ); + registration_server.SaveExtractedFiles( false ); + registration_server.Process(); } - - /* When the update stack, constructed above, is not empty... + /* With the installation record safely compiled, we may + * discard the temporary "release" descriptor from which + * we compiled it. */ - if( stack != NULL ) - { while( stack != NULL ) + dbase.GetRoot()->DeleteChild( reference_hook ); + } + + /* When the update stack, constructed above, is not empty... + */ + if( stack != NULL ) + { while( stack != NULL ) + { + /* ...pop each installation record, which is to be updated, + * off the update stack, in turn... + */ + const char *tarname; + pkgXmlNode *installed; + stack = stack->pop( &installed ); + if( (tarname = installed->GetPropVal( tarname_key, NULL )) != NULL ) { - /* ...pop each installation record, which is to be updated, - * off the update stack, in turn... + /* ...identify its associated installed files manifest, and + * disassociate it from the sysroot of the current installation; + * (note that this automatically deletes the manifest itself, if + * it is associated with no other sysroots). */ - const char *tarname; - pkgXmlNode *installed; - stack = stack->pop( &installed ); - if( (tarname = installed->GetPropVal( tarname_key, NULL )) != NULL ) - { - /* ...identify its associated installed files manifest, and - * disassociate it from the sysroot of the current installation; - * (note that this automatically deletes the manifest itself, if - * it is associated with no other sysroots). - */ - pkgManifest inventory( package_key, tarname ); - inventory.DetachSysRoot( sysroot->GetPropVal( id_key, subsystem ) ); - } - /* Delete the installation record from the current sysroot... - */ - sysroot->DeleteChild( installed ); + pkgManifest inventory( package_key, tarname ); + inventory.DetachSysRoot( sysroot->GetPropVal( id_key, subsystem ) ); } - /* ...and mark the sysroot record as "modified", as a result of - * all preceding updates. + /* Delete the installation record from the current sysroot... */ - sysroot->SetAttribute( modified_key, value_yes ); + sysroot->DeleteChild( installed ); } + /* ...and mark the sysroot record as "modified", as a result of + * all preceding updates. + */ + sysroot->SetAttribute( modified_key, value_yes ); } - - /* Repeat for all packages with an associated setup action... - */ - current = current->next; } - /* ...and finally, report completion of all database updates, while also - * committing all recorded changes to disk storage. + + /* Repeat for all packages with an associated setup action... */ - dmh_notify( DMH_INFO, "%s: installation database updated\n", setup_key ); - dbase.UpdateSystemMap(); + current = current->next; } + /* ...and finally, report completion of all database updates, while also + * committing all recorded changes to disk storage. + */ + dmh_notify( DMH_INFO, "%s: installation database updated\n", setup_key ); + dbase.UpdateSystemMap(); } /* $RCSfile$: end of file */ diff --git a/src/guiexec.cpp b/src/guiexec.cpp index a4ee329..454b85c 100644 --- a/src/guiexec.cpp +++ b/src/guiexec.cpp @@ -3,8 +3,8 @@ * * $Id$ * - * Written by Keith Marshall - * Copyright (C) 2012, 2013, MinGW.org Project + * Written by Keith Marshall + * Copyright (C) 2012, 2013, 2020, MinGW.org Project * * * Implementation of XML data loading services for the mingw-get GUI. @@ -141,10 +141,10 @@ void AppWindowMaker::LoadPackageData( bool force_update ) */ delete pkgData; } - /* Commence loading... */ - if( ! (pkgData = new pkgXmlDocument( dfile ))->IsOk() ) + pkgData = new pkgXmlDocument( dfile ); + if( (pkgData == NULL) || ! pkgData->IsOk() ) /* * ...bailing out on failure to access the initial file. */ @@ -705,7 +705,8 @@ bool AppWindowMaker::ConfirmActionRequest( const char *desc ) * dialogue procedure when appropriate, or simply allowing the * action to proceed, otherwise. */ - return (pkgData->Schedule()->EnumeratePendingActions() > 0) + pkgActionItem *pending = pkgData->Schedule(); + return (pending && pending->EnumeratePendingActions() > 0) ? DialogBox( AppInstance, MAKEINTRESOURCE( IDD_CONFIRMATION ), AppWindow, ConfirmActionDialogue ) == IDOK diff --git a/src/pkgbase.h b/src/pkgbase.h index 7c54f71..e87c0df 100644 --- a/src/pkgbase.h +++ b/src/pkgbase.h @@ -4,8 +4,8 @@ * * $Id$ * - * Written by Keith Marshall - * Copyright (C) 2009-2013, MinGW.org Project + * Written by Keith Marshall + * Copyright (C) 2009-2013, 2020, MinGW.org Project * * * Public interface for the package directory management routines; @@ -74,24 +74,20 @@ class pkgXmlNode : public TiXmlElement inline pkgXmlNode( const pkgXmlNode& src ):TiXmlElement( src ){} /* Accessors... - * - * Note that tinyxml is generally careless about checking for - * possible dereferencing of NULL pointers; thus, many of these - * wrappers include appropriate checks, to prevent this. */ inline const char* GetName() { /* Retrieve the identifying name of the XML tag; * tinyxml calls this the element "value"... */ - return this ? Value() : NULL; + return Value(); } inline pkgXmlNode* GetParent() { /* wxXmlNode provides this equivalant of tinyxml's * Parent() method. */ - return this ? (pkgXmlNode*)(Parent()) : NULL; + return (pkgXmlNode*)(Parent()); } inline pkgXmlNode* GetChildren() { @@ -99,7 +95,7 @@ class pkgXmlNode : public TiXmlElement * the children of an element; it is equivalent to the * FirstChild() method in tinyxml's arsenal. */ - return this ? (pkgXmlNode*)(FirstChild()) : NULL; + return (pkgXmlNode*)(FirstChild()); } inline pkgXmlNode* GetNext() { @@ -107,7 +103,7 @@ class pkgXmlNode : public TiXmlElement * of an element, after the first found by GetChildren(); * it is equivalent to tinyxml's NextSibling(). */ - return this ? (pkgXmlNode*)(NextSibling()) : NULL; + return (pkgXmlNode*)(NextSibling()); } inline const char* GetPropVal( const char* name, const char* subst ) { @@ -115,15 +111,16 @@ class pkgXmlNode : public TiXmlElement * (which substitutes default "subst" text for an omitted property), * but it may be trivially emulated, using the Attribute() method. */ - const char* retval = this ? Attribute( name ) : subst; - return retval ? retval : subst; + const char *retval; + if( (retval = Attribute( name )) == NULL ) return subst; + return retval; } inline pkgXmlNode* AddChild( TiXmlNode *child ) { /* This is wxXmlNode's method for adding a child node, it is * equivalent to tinyxml's LinkEndChild() method. */ - return this ? (pkgXmlNode*)(LinkEndChild( child )) : NULL; + return (pkgXmlNode*)(LinkEndChild( child )); } inline bool DeleteChild( pkgXmlNode *child ) { @@ -132,7 +129,7 @@ class pkgXmlNode : public TiXmlElement * simply use the RemoveChild method here, where for wxXmlNode, we * would use RemoveChild followed by `delete child'. */ - return this ? RemoveChild( child ) : false; + return RemoveChild( child ); } /* Additional methods specific to the application. @@ -141,14 +138,14 @@ class pkgXmlNode : public TiXmlElement { /* Convenience method to retrieve a pointer to the document root. */ - return this ? (pkgXmlNode*)(GetDocument()->RootElement()) : NULL; + return (pkgXmlNode*)(GetDocument()->RootElement()); } inline bool IsElementOfType( const char* tagname ) { /* Confirm if the owner XML node represents a data element * with the specified "tagname". */ - return this ? strcmp( GetName(), tagname ) == 0 : false; + return strcmp( GetName(), tagname ) == 0; } /* Methods to determine which packages should be displayed @@ -269,13 +266,13 @@ class pkgActionItem unsigned long SetAuthorities( pkgActionItem* ); inline unsigned long HasAttribute( unsigned long required ) { - return (this != NULL) ? flags & required : 0UL; + return flags & required; } pkgActionItem* GetReference( pkgXmlNode* ); pkgActionItem* GetReference( pkgActionItem& ); pkgActionItem* Schedule( unsigned long, pkgActionItem& ); inline pkgActionItem* SuppressRedundantUpgrades( void ); - inline unsigned long CancelScheduledAction( void ); + inline void CancelScheduledAction( void ); inline void SetPrimary( pkgActionItem* ); /* Method to enumerate and identify pending changes, @@ -293,13 +290,13 @@ class pkgActionItem { /* Mark a package as the selection for a specified action. */ - if (this != NULL) selection[ opt ] = pkg; + selection[ opt ] = pkg; } inline pkgXmlNode* Selection( int mode = to_install ) { /* Retrieve the package selection for a specified action. */ - return (this != NULL) ? selection[ mode ] : NULL; + return selection[ mode ]; } void ConfirmInstallationStatus(); @@ -454,7 +451,7 @@ class pkgXmlDocument : public TiXmlDocument /* Method to execute a sequence of scheduled actions. */ - inline void ExecuteActions(){ actions->Execute(); } + inline void ExecuteActions(){ if( actions ) actions->Execute(); } /* Method to clear the list of scheduled actions. */ diff --git a/src/pkgdata.cpp b/src/pkgdata.cpp index 841e07b..7c9ff45 100644 --- a/src/pkgdata.cpp +++ b/src/pkgdata.cpp @@ -3,8 +3,8 @@ * * $Id$ * - * Written by Keith Marshall - * Copyright (C) 2012, 2013, MinGW.org Project + * Written by Keith Marshall + * Copyright (C) 2012, 2013, 2020, MinGW.org Project * * * Implementation of the classes and methods required to support the @@ -1085,10 +1085,9 @@ void AppWindowMaker::UpdatePackageMenuBindings() * this is also a convenient point to consider activation of the * "Apply Changes" capability. */ - EnableMenuItem( menu, IDM_REPO_APPLY, - (pkgData->Schedule()->EnumeratePendingActions() > 0) ? MF_ENABLED - : MF_GRAYED - ); + pkgActionItem *pending = pkgData->Schedule(); + unsigned long count = (pending != NULL) ? pending->EnumeratePendingActions() : 0UL; + EnableMenuItem( menu, IDM_REPO_APPLY, (count > 0UL) ? MF_ENABLED : MF_GRAYED ); } inline void AppWindowMaker::MarkSchedule( pkgActionItem *pending_actions ) @@ -1191,11 +1190,11 @@ void AppWindowMaker::Schedule } } -inline unsigned long pkgActionItem::CancelScheduledAction( void ) +inline void pkgActionItem::CancelScheduledAction( void ) { /* Helper method to mark a scheduled action as "cancelled". */ - return (this != NULL) ? (flags &= ~ACTION_MASK) : 0UL; + flags &= ~ACTION_MASK; } void AppWindowMaker::UnmarkSelectedPackage( void ) @@ -1223,7 +1222,9 @@ void AppWindowMaker::UnmarkSelectedPackage( void ) * ...search the action schedule, for an action associated with * this package, if any, and cancel it. */ - pkgData->Schedule()->GetReference( pkg )->CancelScheduledAction(); + pkgActionItem *pkgref = pkgData->Schedule(); + if( (pkgref != NULL) && ((pkgref = pkgref->GetReference( pkg )) != NULL) ) + pkgref->CancelScheduledAction(); /* The scheduling state for packages shown in the list view * may have changed, so refresh the icon associations and the @@ -1401,74 +1402,72 @@ unsigned long pkgActionItem::EnumeratePendingActions( int classified ) * scheduled action list. */ unsigned long count = 0; - if( this != NULL ) - { - /* Regardless of the position of the 'this' pointer, - * within the list of scheduled actions... + + /* Regardless of the position of the 'this' pointer, + * within the list of scheduled actions... + */ + pkgActionItem *item = this; + while( item->prev != NULL ) + /* + * ...we want to get a reference to the first + * item in the list. */ - pkgActionItem *item = this; - while( item->prev != NULL ) - /* - * ...we want to get a reference to the first - * item in the list. - */ - item = item->prev; + item = item->prev; - /* Now, working through the list... + /* Now, working through the list... + */ + while( item != NULL ) + { + /* ...note items with any scheduled action... */ - while( item != NULL ) + int action; + if( (action = item->flags & ACTION_MASK) != 0 ) { - /* ...note items with any scheduled action... + /* ...and, when one is found, (noting that ACTION_UPGRADE may + * also be considered as a special case of ACTION_INSTALL)... */ - int action; - if( (action = item->flags & ACTION_MASK) != 0 ) + if( (action == classified) + || ((action == ACTION_UPGRADE) && (classified == ACTION_INSTALL)) ) { - /* ...and, when one is found, (noting that ACTION_UPGRADE may - * also be considered as a special case of ACTION_INSTALL)... + /* ...and it matches the classification in which + * we are interested, then we retrieve the tarname + * for the related package... */ - if( (action == classified) - || ((action == ACTION_UPGRADE) && (classified == ACTION_INSTALL)) ) + pkgXmlNode *selected = (classified & ACTION_REMOVE) + ? item->Selection( to_remove ) + : item->Selection(); + const char *notification = (selected != NULL) + ? selected->GetPropVal( tarname_key, NULL ) + : NULL; + if( notification != NULL ) { - /* ...and it matches the classification in which - * we are interested, then we retrieve the tarname - * for the related package... - */ - pkgXmlNode *selected = (classified & ACTION_REMOVE) - ? item->Selection( to_remove ) - : item->Selection(); - const char *notification = (selected != NULL) - ? selected->GetPropVal( tarname_key, NULL ) - : NULL; - if( notification != NULL ) - { - /* ...and, provided it is valid, we append it to - * the DMH driven dialogue in which the enumeration - * is being reported... - */ - dmh_printf( "%s\n", notification ); - /* - * ...and include it in the accumulated count... - */ - ++count; - } - } - else if( (classified == 0) - /* - * ...otherwise, when we aren't interested in any particular - * class of action regardless of classification... + /* ...and, provided it is valid, we append it to + * the DMH driven dialogue in which the enumeration + * is being reported... */ - || ((classified == ACTION_UNSUCCESSFUL) && ((flags & classified) != 0)) ) + dmh_printf( "%s\n", notification ); /* - * ...or when we are checking for unsuccessful actions, we - * count all those which are found, either unclassified, or - * marked as unsuccessful, respectively. + * ...and include it in the accumulated count... */ ++count; + } } - /* ...then move on, to consider the next entry, if any. - */ - item = item->next; + else if( (classified == 0) + /* + * ...otherwise, when we aren't interested in any particular + * class of action regardless of classification... + */ + || ((classified == ACTION_UNSUCCESSFUL) && ((flags & classified) != 0)) ) + /* + * ...or when we are checking for unsuccessful actions, we + * count all those which are found, either unclassified, or + * marked as unsuccessful, respectively. + */ + ++count; } + /* ...then move on, to consider the next entry, if any. + */ + item = item->next; } /* Ultimately, return the count of pending actions, * as noted while processing the above loop. diff --git a/src/pkgdeps.cpp b/src/pkgdeps.cpp index 678347d..c23164f 100644 --- a/src/pkgdeps.cpp +++ b/src/pkgdeps.cpp @@ -3,8 +3,8 @@ * * $Id$ * - * Written by Keith Marshall - * Copyright (C) 2009, 2010, 2011, 2012, MinGW Project + * Written by Keith Marshall + * Copyright (C) 2009-2012, 2020, MinGW.org Project * * * Implementation of the package dependency resolver method, of the @@ -859,47 +859,42 @@ void pkgActionItem::ApplyBounds( pkgXmlNode *release, const char *bounds ) pkgActionItem *pkgActionItem::GetReference( pkgXmlNode *package ) { - /* Method to locate a scheduled action, if any, which relates - * to a specified package. + /* Method to locate a scheduled action, if any, which relates to + * a specified package. Assume that the initial 'this' pointer is + * closer to the end, than to the beginning of the current list of + * scheduled actions, (which MUST NOT be empty), and... */ - if( this != NULL ) - { - /* The schedule of actions is not empty. Assume that the - * initial 'this' pointer is closer to the end, than to the - * beginning of the list of scheduled actions, and... + pkgActionItem *item = this; + while( item->next != NULL ) + /* + * ...advance, to locate the very last entry in the list. */ - pkgActionItem *item = this; - while( item->next != NULL ) - /* - * ...advance, to locate the very last entry in the list. - */ - item = item->next; + item = item->next; - /* Now, working backward toward the beginning of the list... + /* Now, working backward toward the beginning of the list... + */ + while( item != NULL ) + { + /* ...identify a "release" specification associated with + * each action item in turn... */ - while( item != NULL ) + pkgXmlNode *ref = item->Selection(); + if( (ref != NULL) || ((ref = item->Selection( to_remove )) != NULL) ) { - /* ...identify a "release" specification associated with - * each action item in turn... + /* ...convert this to an actual component package, or + * full package, reference... */ - pkgXmlNode *ref = item->Selection(); - if( (ref != NULL) || ((ref = item->Selection( to_remove )) != NULL) ) - { - /* ...convert this to an actual component package, or - * full package, reference... - */ - while( ref->IsElementOfType( release_key ) ) - ref = ref->GetParent(); + while( ref->IsElementOfType( release_key ) ) + ref = ref->GetParent(); - /* ...and, if it matches the search target, return it. - */ - if( ref == package ) return item; - } - /* ...or, when we haven't yet found a matching package, - * try the preceding scheduled action item, if any. + /* ...and, if it matches the search target, return it. */ - item = item->prev; + if( ref == package ) return item; } + /* ...or, when we haven't yet found a matching package, + * try the preceding scheduled action item, if any. + */ + item = item->prev; } /* If we fall through to here, then we found no action to be * performed on the specified package; report accordingly. @@ -945,12 +940,6 @@ pkgActionItem* pkgXmlDocument::Schedule( unsigned long action, const char* name * dependencies for the package specified by "name", honouring * any appended version bounds specified for the parent. */ - if( this == NULL ) - /* - * An unassigned XML database document cannot have any - * assigned action; bail out, with appropriate status. - */ - return NULL; /* We may call this method without any assigned package name, * in which case, we interpret it as a request to return the @@ -1184,8 +1173,8 @@ pkgActionItem* pkgXmlDocument::Schedule( unsigned long action, const char* name ResolveDependencies( upgrade, Schedule( action, latest )); } } - - if( (component = component->FindNextAssociate( component_key )) != NULL ) + if( (component != NULL) + && ((component = component->FindNextAssociate( component_key )) != NULL) ) /* * When evaluating a component-package, we extend our * evaluation, to consider for any further components of diff --git a/src/pkgexec.cpp b/src/pkgexec.cpp index ef86aac..c1d8f75 100644 --- a/src/pkgexec.cpp +++ b/src/pkgexec.cpp @@ -209,16 +209,13 @@ pkgActionItem::Append( pkgActionItem *item ) * being appended to the list, but the implementation preserves * integrity of any following list items, thus also fulfilling * an "insert after this" function. - */ - if( this == NULL ) - /* - * No list exists yet; - * return "item" as first and only entry in new list. - */ - return item; - - /* Ensure "item" physically exists, or if not, create a generic - * placeholder in which to construct it... + * + * Note that this method must NEVER be invoked with a NULL "this" + * pointer; however, conformance with this requirement CANNOT be + * verified here, so MUST be verified at the call site. + * + * The "item" to be added must physically exist; if not, we create + * a generic placeholder in which to construct it... */ if( (item == NULL) && ((item = new pkgActionItem()) == NULL) ) /* @@ -249,17 +246,10 @@ pkgActionItem* pkgActionItem::Insert( pkgActionItem *item ) { /* Add an "item" to an ActionItems list, inserting it immediately - * before the item referenced by the "this" pointer. - */ - if( this == NULL ) - /* - * No list exists yet; - * return "item" as first and only entry in new list. - */ - return item; - - /* Ensure "item" physically exists, or if not, create a generic - * placeholder in which to construct it... + * before the item referenced by the "this" pointer, (which should + * NEVER be NULL, but this must be verified at the call site). The + * "item" to be added must physically exist; if not, we create a + * generic placeholder in which to construct it... */ if( (item == NULL) && ((item = new pkgActionItem()) == NULL) ) /* @@ -451,12 +441,12 @@ pkgActionItem* pkgXmlDocument::Schedule * the action list, (or at the end of the list if no ranking * position is specified)... */ - pkgActionItem *ref = rank ? rank : actions; + pkgActionItem *ref = rank ? rank : actions ? actions : &item; /* If we already have a prior matching item... */ pkgActionItem *prior; - if( (prior = actions->GetReference( item )) != NULL ) + if( (prior = (actions ? actions->GetReference( item ) : NULL)) != NULL ) { /* ...then, when the current request refers to a primary action, * we update the already scheduled request to reflect this... @@ -486,15 +476,14 @@ pkgActionItem* pkgXmlDocument::Schedule /* ...and, when successfully raised, add it to the task list... */ if( rank ) - /* - * ...at the specified ranking position, if any... + /* ...at the specified ranking position, if any... */ return rank->Insert( ref ); else /* ...otherwise, at the end of the list. */ - return actions = actions->Append( ref ); + return actions = actions ? actions->Append( ref ) : ref; } /* If we get to here, then no new action was scheduled; we simply @@ -517,147 +506,145 @@ int reinstall_action_scheduled( pkgActionItem *package ) void pkgActionItem::Execute( bool with_download ) { - if( this != NULL ) - { pkgActionItem *current = this; - bool init_rites_pending = true; - while( current->prev != NULL ) current = current->prev; - - /* Unless normal operations have been suppressed by the - * --print-uris option, (in order to obtain a list of all - * package URIs which the operation would access)... + pkgActionItem *current = this; + bool init_rites_pending = true; + while( current->prev != NULL ) current = current->prev; + + /* Unless normal operations have been suppressed by the + * --print-uris option, (in order to obtain a list of all + * package URIs which the operation would access)... + */ + if( pkgOptions()->Test( OPTION_PRINT_URIS ) < OPTION_PRINT_URIS ) + do { + /* ...we initiate any download requests which may + * be necessary to fetch all required archives into + * the local package cache. + */ + if( with_download ) + DownloadArchiveFiles( current ); + } while( SetAuthorities( current ) > 0 ); + + else while( current != NULL ) + { + /* The --print-uris option is in effect: we simply loop + * over all packages with an assigned action, printing + * the associated download URI for each; (note that this + * will print the URI regardless of prior existence of + * the associated package in the local cache). */ - if( pkgOptions()->Test( OPTION_PRINT_URIS ) < OPTION_PRINT_URIS ) - do { - /* ...we initiate any download requests which may - * be necessary to fetch all required archives into - * the local package cache. - */ - if( with_download ) - DownloadArchiveFiles( current ); - } while( SetAuthorities( current ) > 0 ); - - else while( current != NULL ) - { - /* The --print-uris option is in effect: we simply loop - * over all packages with an assigned action, printing - * the associated download URI for each; (note that this - * will print the URI regardless of prior existence of - * the associated package in the local cache). - */ - current->PrintURI( current->Selection()->ArchiveName() ); - current = current->next; - } + current->PrintURI( current->Selection()->ArchiveName() ); + current = current->next; + } - /* If the --download-only option is in effect, then we have - * nothing more to do... + /* If the --download-only option is in effect, then we have + * nothing more to do... + */ + if( pkgOptions()->Test( OPTION_DOWNLOAD_ONLY ) != OPTION_DOWNLOAD_ONLY ) + { + /* ...otherwise... */ - if( pkgOptions()->Test( OPTION_DOWNLOAD_ONLY ) != OPTION_DOWNLOAD_ONLY ) + while( current != NULL ) { - /* ...otherwise... + /* ...processing only those packages with assigned actions... */ - while( current != NULL ) + if( (current->flags & ACTION_MASK) != 0 ) { - /* ...processing only those packages with assigned actions... + /* ...print a notification of the installation process to + * be performed, identifying the package to be processed. */ - if( (current->flags & ACTION_MASK) != 0 ) + const char *tarname; + pkgXmlNode *ref = current->Selection(); + if( (tarname = ref->GetPropVal( tarname_key, NULL )) == NULL ) { - /* ...print a notification of the installation process to - * be performed, identifying the package to be processed. - */ - const char *tarname; - pkgXmlNode *ref = current->Selection(); - if( (tarname = ref->GetPropVal( tarname_key, NULL )) == NULL ) - { - ref = current->Selection( to_remove ); - tarname = ref->GetPropVal( tarname_key, value_unknown ); - } - dmh_printf( "%s: %s\n", reinstall_action_scheduled( current ) - ? "reinstall" : action_name( current->flags & ACTION_MASK ), - tarname - ); - - /* Package pre/post processing scripts may need to - * refer to the sysroot path for the package; place - * a copy in the environment, to facilitate this. - */ - pkgSpecs lookup( tarname ); - ref = ref->GetSysRoot( lookup.GetSubSystemName() ); - const char *path = ref->GetPropVal( pathname_key, NULL ); - if( path != NULL ) - { - /* Format the sysroot path into an environment variable - * assignment specification; note that the recorded path - * name is likely to include macros such as "%R", so we - * filter it through mkpath(), to expand them. - */ - const char *nothing = ""; - char varspec_template[9 + strlen( path )]; - sprintf( varspec_template, "SYSROOT=%s", path ); - char varspec[mkpath( NULL, varspec_template, nothing, NULL )]; - mkpath( varspec, varspec_template, nothing, NULL ); - pkgPutEnv( PKG_PUTENV_DIRSEP_MSW, varspec ); - } + ref = current->Selection( to_remove ); + tarname = ref->GetPropVal( tarname_key, value_unknown ); + } + dmh_printf( "%s: %s\n", reinstall_action_scheduled( current ) + ? "reinstall" : action_name( current->flags & ACTION_MASK ), + tarname + ); - /* Check for any outstanding requirement to invoke the - * "self upgrade rites" process, so that we may install an - * upgrade for mingw-get itself... + /* Package pre/post processing scripts may need to + * refer to the sysroot path for the package; place + * a copy in the environment, to facilitate this. + */ + pkgSpecs lookup( tarname ); + ref = ref->GetSysRoot( lookup.GetSubSystemName() ); + const char *path = ref->GetPropVal( pathname_key, NULL ); + if( path != NULL ) + { + /* Format the sysroot path into an environment variable + * assignment specification; note that the recorded path + * name is likely to include macros such as "%R", so we + * filter it through mkpath(), to expand them. */ - if( init_rites_pending ) - /* - * ...discontinuing the check once this has been completed, - * since it need not be performed more than once. - */ - init_rites_pending = self_upgrade_rites( tarname ); + const char *nothing = ""; + char varspec_template[9 + strlen( path )]; + sprintf( varspec_template, "SYSROOT=%s", path ); + char varspec[mkpath( NULL, varspec_template, nothing, NULL )]; + mkpath( varspec, varspec_template, nothing, NULL ); + pkgPutEnv( PKG_PUTENV_DIRSEP_MSW, varspec ); + } - /* If we are performing an upgrade... - */ - if( ((current->flags & ACTION_MASK) == ACTION_UPGRADE) + /* Check for any outstanding requirement to invoke the + * "self upgrade rites" process, so that we may install an + * upgrade for mingw-get itself... + */ + if( init_rites_pending ) /* - * ...and the latest version of the package is already installed... + * ...discontinuing the check once this has been completed, + * since it need not be performed more than once. */ - && (current->Selection() == current->Selection( to_remove )) + init_rites_pending = self_upgrade_rites( tarname ); + + /* If we are performing an upgrade... + */ + if( ((current->flags & ACTION_MASK) == ACTION_UPGRADE) + /* + * ...and the latest version of the package is already installed... + */ + && (current->Selection() == current->Selection( to_remove )) + /* + * ...and the `--reinstall' option hasn't been specified... + */ + && (pkgOptions()->Test( OPTION_REINSTALL ) == 0) ) /* - * ...and the `--reinstall' option hasn't been specified... + * ...then simply report the up-to-date status... + */ + dmh_notify( DMH_INFO, "package %s is up to date\n", tarname ); + + else + { /* ...otherwise, proceed to perform remove and install + * operations, as appropriate. */ - && (pkgOptions()->Test( OPTION_REINSTALL ) == 0) ) - /* - * ...then simply report the up-to-date status... + if( reinstall_action_scheduled( current ) + || ((current->flags & ACTION_REMOVE) == ACTION_REMOVE) ) + { + /* The selected package has been marked for removal, either + * explicitly, or as an implicit prerequisite for upgrade, or + * in preparation for reinstallation. */ - dmh_notify( DMH_INFO, "package %s is up to date\n", tarname ); + pkgRemove( current ); + } - else - { /* ...otherwise, proceed to perform remove and install - * operations, as appropriate. + if( (current->flags & ACTION_INSTALL) == ACTION_INSTALL ) + { + /* The selected package has been marked for installation, + * either explicitly, or implicitly to complete a package upgrade. */ + pkgXmlNode *tmp = current->Selection( to_remove ); if( reinstall_action_scheduled( current ) - || ((current->flags & ACTION_REMOVE) == ACTION_REMOVE) ) - { - /* The selected package has been marked for removal, either - * explicitly, or as an implicit prerequisite for upgrade, or - * in preparation for reinstallation. - */ - pkgRemove( current ); - } - - if( (current->flags & ACTION_INSTALL) == ACTION_INSTALL ) - { - /* The selected package has been marked for installation, - * either explicitly, or implicitly to complete a package upgrade. - */ - pkgXmlNode *tmp = current->Selection( to_remove ); - if( reinstall_action_scheduled( current ) - || ((current->flags & ACTION_MASK) == ACTION_UPGRADE) ) - current->selection[ to_remove ] = NULL; - pkgInstall( current ); - current->selection[ to_remove ] = tmp; - } + || ((current->flags & ACTION_MASK) == ACTION_UPGRADE) ) + current->selection[ to_remove ] = NULL; + pkgInstall( current ); + current->selection[ to_remove ] = tmp; } } - /* Proceed to the next package, if any, with scheduled actions. - */ - pkgSpinWait::Report( "Processing... (%c)", pkgSpinWait::Indicator() ); - current = current->next; } + /* Proceed to the next package, if any, with scheduled actions. + */ + pkgSpinWait::Report( "Processing... (%c)", pkgSpinWait::Indicator() ); + current = current->next; } } } diff --git a/src/pkginst.cpp b/src/pkginst.cpp index 3789da5..ddd241c 100644 --- a/src/pkginst.cpp +++ b/src/pkginst.cpp @@ -227,9 +227,10 @@ void pkgManifest::AddEntry( const char *key, const char *pathname ) * entries to the tracked inventory of package content. * * Tracking is enabled only if the manifest structure is valid, - * AND an inventory table has been allocated... + * (which must have been verified by the caller), AND an inventory + * table has been allocated... */ - if( (this != NULL) && (inventory != NULL) ) + if( inventory != NULL ) { /* ...in which case we allocate a new tracking record, with * "dir" or "file" reference key as appropriate, fill it out diff --git a/src/pkgunst.cpp b/src/pkgunst.cpp index e40c829..c832da3 100644 --- a/src/pkgunst.cpp +++ b/src/pkgunst.cpp @@ -170,7 +170,7 @@ pkgXmlNode *pkgManifest::GetSysRootReference( const char *key ) * a reference to any sysroot which claims it, returning * a pointer to the first such reference found. */ - if( (this != NULL) && (manifest != NULL) && (key != NULL) ) + if( (manifest != NULL) && (key != NULL) ) { /* We appear to have a valid manifest, and a valid sysroot * key to match; locate this manifest's first, (and nominally diff --git a/src/setup.cpp b/src/setup.cpp index b7a6a45..b63324e 100644 --- a/src/setup.cpp +++ b/src/setup.cpp @@ -263,14 +263,6 @@ EXTERN_C void dmh_init( const dmh_class subsystem, const char *progname ) dmh = new dmhTypeGUI( strdup( progname ) ); } -inline unsigned long pkgSetupAction::HasAttribute( unsigned long mask ) -{ - /* Helper function to examine the flags within a setup action - * item, checking for the presence of a specified attribute. - */ - return (this != NULL) ? mask & flags : 0UL; -} - void pkgSetupAction::ClearAllActions( void ) { /* Routine to clear an entire list of setup action items, diff --git a/src/setup.h b/src/setup.h index 9463c0c..215be45 100644 --- a/src/setup.h +++ b/src/setup.h @@ -4,8 +4,8 @@ * * $Id$ * - * Written by Keith Marshall - * Copyright (C) 2013, MinGW.org Project + * Written by Keith Marshall + * Copyright (C) 2013, 2020, MinGW.org Project * * * Resource ID definitions and class declarations which are specific @@ -96,12 +96,12 @@ class pkgSetupAction * specifically to the requirements of the setup tool. */ public: - inline unsigned long HasAttribute( unsigned long ); pkgSetupAction( pkgSetupAction *, const char *, const char * = 0 ); + inline unsigned long HasAttribute( unsigned long mask){ return flags & mask; } inline void DownloadArchiveFiles( void ); inline int UnpackScheduledArchives( void ); - void ClearAllActions( void ); void UpdateDatabase( pkgXmlDocument & ); + void ClearAllActions( void ); private: unsigned long flags;