Re: msi: Remove tentative definition of static array with no size specifier

[James Hawkins]

On Wed, Apr 2, 2008 at 5:07 PM, Andrew Talbot
 wrote:
> Whereas for a non-static array a tentative definition does not require a
>  size to be specified, for example:
> char ar[];            /* tentative definition */
>
> char ar[] = "hello";  /* actual definition */
>
> strictly, when the static storage specifier is applied, the size must be
>  specified:
> static char ar[5];           /* tentative definition */
>
> static char ar[5] = "hello"; /* actual definition */
>
> This patch avoids using a magic number by moving the definition of an array
>  and adding forward declarations for the functions affected, instead.
> -- Andy.
>
> Changelog:
>     msi: Remove tentative declaration of static array with no size specifier.
>
> diff --git a/dlls/msi/action.c b/dlls/msi/action.c
>  index 982c9f0..8cc36a0 100644
>  --- a/dlls/msi/action.c
>  +++ b/dlls/msi/action.c
>  @@ -47,6 +47,64 @@ WINE_DEFAULT_DEBUG_CHANNEL(msi);
>   static UINT ACTION_ProcessExecSequence(MSIPACKAGE *package, BOOL UIran);
>   static UINT ACTION_ProcessUISequence(MSIPACKAGE *package);
>   static UINT ACTION_PerformActionSequence(MSIPACKAGE *package, UINT seq, BOOL UI);
>  +static UINT ACTION_AllocateRegistrySpace( MSIPACKAGE *package );
>  +static UINT ACTION_BindImage( MSIPACKAGE *package );
>  +static UINT ACTION_CostFinalize(MSIPACKAGE *package);
>  +static UINT ACTION_CostInitialize(MSIPACKAGE *package);
>  +static UINT ACTION_CreateFolders(MSIPACKAGE *package);
>  +static UINT ACTION_CreateShortcuts(MSIPACKAGE *package);
>  +static UINT ACTION_DeleteServices( MSIPACKAGE *package );
>  +static UINT ACTION_ExecuteAction(MSIPACKAGE *package);
>  +static UINT ACTION_FileCost(MSIPACKAGE *package);
>  +static UINT ACTION_InstallExecute(MSIPACKAGE *package);
>  +static UINT ACTION_InstallFinalize(MSIPACKAGE *package);
>  +static UINT ACTION_InstallInitialize(MSIPACKAGE *package);
>  +static UINT ACTION_InstallSFPCatalogFile( MSIPACKAGE *package );
>  +static UINT ACTION_InstallValidate(MSIPACKAGE *package);
>  +static UINT ACTION_IsolateComponents( MSIPACKAGE *package );
>  +static UINT ACTION_LaunchConditions(MSIPACKAGE *package);
>  +static UINT ACTION_MigrateFeatureStates( MSIPACKAGE *package );
>  +static UINT ACTION_MoveFiles( MSIPACKAGE *package );
>  +static UINT ACTION_MsiPublishAssemblies( MSIPACKAGE *package );
>  +static UINT ACTION_MsiUnpublishAssemblies( MSIPACKAGE *package );
>  +static UINT ACTION_InstallODBC( MSIPACKAGE *package );
>  +static UINT ACTION_InstallServices( MSIPACKAGE *package );
>  +static UINT ACTION_PatchFiles( MSIPACKAGE *package );
>  +static UINT ACTION_ProcessComponents(MSIPACKAGE *package);
>  +static UINT ACTION_PublishComponents(MSIPACKAGE *package);
>  +static UINT ACTION_PublishFeatures(MSIPACKAGE *package);
>  +static UINT ACTION_PublishProduct(MSIPACKAGE *package);
>  +static UINT ACTION_RegisterComPlus( MSIPACKAGE *package );
>  +static UINT ACTION_RegisterProduct(MSIPACKAGE *package);
>  +static UINT ACTION_RegisterTypeLibraries(MSIPACKAGE *package);
>  +static UINT ACTION_RegisterUser(MSIPACKAGE *package);
>  +static UINT ACTION_RemoveDuplicateFiles( MSIPACKAGE *package );
>  +static UINT ACTION_RemoveEnvironmentStrings( MSIPACKAGE *package );
>  +static UINT ACTION_RemoveExistingProducts( MSIPACKAGE *package );
>  +static UINT ACTION_RemoveFolders( MSIPACKAGE *package );
>  +static UINT ACTION_RemoveIniValues( MSIPACKAGE *package );
>  +static UINT ACTION_RemoveODBC( MSIPACKAGE *package );
>  +static UINT ACTION_RemoveRegistryValues( MSIPACKAGE *package );
>  +static UINT ACTION_RemoveShortcuts( MSIPACKAGE *package );
>  +static UINT ACTION_ResolveSource(MSIPACKAGE* package);
>  +static UINT ACTION_RMCCPSearch( MSIPACKAGE *package );
>  +static UINT ACTION_SelfRegModules(MSIPACKAGE *package);
>  +static UINT ACTION_SelfUnregModules( MSIPACKAGE *package );
>  +static UINT ACTION_StartServices( MSIPACKAGE *package );
>  +static UINT ACTION_StopServices( MSIPACKAGE *package );
>  +static UINT ACTION_UnpublishComponents( MSIPACKAGE *package );
>  +static UINT ACTION_UnpublishFeatures(MSIPACKAGE *package);
>  +static UINT ACTION_UnregisterClassInfo( MSIPACKAGE *package );
>  +static UINT ACTION_UnregisterComPlus( MSIPACKAGE *package );
>  +static UINT ACTION_UnregisterExtensionInfo( MSIPACKAGE *package );
>  +static UINT ACTION_UnregisterFonts( MSIPACKAGE *package );
>  +static UINT ACTION_UnregisterMIMEInfo( MSIPACKAGE *package );
>  +static UINT ACTION_UnregisterProgIdInfo( MSIPACKAGE *package );
>  +static UINT ACTION_UnregisterTypeLibraries( MSIPACKAGE *package );
>  +static UINT ACTION_ValidateProductID( MSIPACKAGE *package );
>  +static UINT ACTION_WriteEnvironmentStrings( MSIPACKAGE *package );
>  +static UINT ACTION_WriteIniValues(MSIPACKAGE *package);
>  +static UINT ACTION_WriteRegistryValues(MSIPACKAGE *package);

I object.  Also, RFCs should be sent to wine-devel, not wine-patches.

msi: Remove tentative definition of static array with no size specifier

[Andrew Talbot]

James Hawkins wrote:
> I object.  Also, RFCs should be sent to wine-devel, not wine-patches.

I was submitting a patch with a prelude explaining why, not making a request
for comment. But on what grounds are you objecting?

msi: Remove tentative definition of static array with no size specifier

[James Hawkins]

On Wed, Apr 2, 2008 at 5:19 PM, Andrew Talbot
 wrote:
> James Hawkins wrote:
> > I object.  Also, RFCs should be sent to wine-devel, not wine-patches.
>
> I was submitting a patch with a prelude explaining why, not making a request
>  for comment. But on what grounds are you objecting?

It's ugly.  What warning are you trying to fix?

msi: Remove tentative definition of static array with no size specifier

[Andrew Talbot]

James Hawkins wrote:
> It's ugly.  What warning are you trying to fix?

Although I imagine that gcc doesn't do anything particularly adverse as a
result of the existing code, if the pedantic switch were applied it would
cause a message of the following type to be generated.

action.c:236: error: array size missing in ?StandardActions?

I believe it is also likely to show up under lint-like tools and I believe
it is actually an error, though compilers are not required to generate any
message, apparently. I couldn't say whether the resultant behaviour is
undefined, implemenation defined, or what. And I don't know whether gcc
places any surplus baggage in any segment as a result. The fix was just to
make the code correct and hence more portable.

msi: Remove tentative definition of static array with no size specifier

[James Hawkins]

On Wed, Apr 2, 2008 at 5:40 PM, Andrew Talbot
 wrote:
> James Hawkins wrote:
> > It's ugly.  What warning are you trying to fix?
>
> Although I imagine that gcc doesn't do anything particularly adverse as a
>  result of the existing code, if the pedantic switch were applied it would
>  cause a message of the following type to be generated.
> action.c:236: error: array size missing in ?StandardActions?
>
> I believe it is also likely to show up under lint-like tools and I believe
>  it is actually an error, though compilers are not required to generate any
>  message, apparently. I couldn't say whether the resultant behaviour is
>  undefined, implemenation defined, or what. And I don't know whether gcc
>  places any surplus baggage in any segment as a result. The fix was just to
>  make the code correct and hence more portable.

That's fine, but it's not worth it to me, and I'm pretty sure Julliard
won't accept it either.

msi: Remove tentative definition of static array with no size specifier

[Andrew Talbot]

James Hawkins wrote:
> That's fine, but it's not worth it to me, and I'm pretty sure Julliard
> won't accept it either.

I understand and suspect you are right. Maybe I should have made an RFC
rather than opting for trial by patch. :)

Thanks for your comments.

msi: Remove tentative definition of static array with no size specifier

[Alexandre Julliard]

Andrew Talbot  writes:
> James Hawkins wrote:
> > That's fine, but it's not worth it to me, and I'm pretty sure Julliard
> > won't accept it either.
>
> I understand and suspect you are right. Maybe I should have made an RFC
> rather than opting for trial by patch. :)

I think it's worth fixing, but it's easier to do by avoiding the need
for the forward declaration, there's only one place that uses it.

msi: Remove tentative definition of static array with no size specifier

[Robert Shearman]

Andrew Talbot wrote:
> James Hawkins wrote:
> > It's ugly.  What warning are you trying to fix?
>
> Although I imagine that gcc doesn't do anything particularly adverse as a
> result of the existing code, if the pedantic switch were applied it would
> cause a message of the following type to be generated.
> action.c:236: error: array size missing in ?StandardActions?
>
> I believe it is also likely to show up under lint-like tools and I believe
> it is actually an error, though compilers are not required to generate any
> message, apparently. I couldn't say whether the resultant behaviour is
> undefined, implemenation defined, or what. And I don't know whether gcc
> places any surplus baggage in any segment as a result. The fix was just to
> make the code correct and hence more portable.

This also causes problems when compiling with msvc:
> make[1]: Entering directory `/home/rob/wine-msvc/dlls/msi'
> CC action.o
> action.c
> action.c(236) : error C2133: 'StandardActions' : unknown size

It also might affect compilers on other platforms that Wine might be 
ported to. As Andy says, it's non-standard behaviour and so could break 
even in future versions of gcc.

I think the patch could be changed so that very few, if any, forward 
declarations would need to be made simply by moving 
ACTION_HandleStandardAction to after StandardActions is initialised.