Bill Paul [mailto:wp...@windriver.com] wrote:
]Sent: Friday, July 10, 2015 05:55 PM
]To: Jordan Justen
]Cc: edk2-devel@lists.sourceforge.net
]Subject: Re: [edk2] [PATCH] Avoid "variable set but not used" warning from GCC.
]
]Of all the gin joints in all the towns in all the world, Jordan Justen had to
]walk into mine at 11:40:01 on Friday 10 July 2015 and say:
]
]> > Be aware too that not every project uses the exact same rules for patch
]> > submissions, so don't be surprised if new (or infrequent) contributors
]> > can't immediately decipher your particular secret handshake.
]>
]> True, but we are trying to follow some fairly common practices.
]
]Well, at the time, so was I. :)
]
]> You'd like us to just accept your contributions as is, and not provide
]> any feedback? I don't think that review feedback amounts to a secret
]> handshake.
]
]Feedback on the _content_ of contributions is perfectly fine.
]
]The "secret handshake" refers to the contribution _form_. If adherence to a
]particular submission format (i.e. subject line contents, markups) is that
]important, consider having a script to generate them, so that at least first-
]timers (or infrequent contributors who might miss rule changes) will have a
]better chance of getting it right. (I've seen people say: "I use a script to
]process commits so if you don't format them exactly right, they may be
]ignored.")
]
]> > > > > We use '//' comments in code.
]> > > > >
]> > > > > Coding Standards Spec, Section "5. 4.2 Internal Comments":
]> > > > >
]> > > > > "For internal code comments, use C++ style (//) comment lines."
]> > > >
]> > > > Yes, I did not miss that,
]> > >
]> > > Well, I guess you could have tweaked it youself rather than requiring
]> > > a repost. (Like the patch subject.)
]> >
]> > Let me be clear that it doesn't matter me what any coding convention
]> > says, it doesn't matter that the C spec allows it, and it doesn't matter
]> > to me how many might advocate it, what arguments they put forth to
]> > support it or in how much esteem they are held: I will never, ever, EVER
]> > use C++-style comments in C code.
]>
]> You shouldn't expect a project to be receptive to your contributions
]> if you are hostile towards its coding standards. What would be your
]> reaction if someone submitted a patch to your project with c++
]> comments in a C file?
]
]I would let it slide, because in this specific case it really doesn't matter.
]
]> I would think it is more constructive to try to open a discussion on
]> having the coding standards changed.
]
]I think the odds of that happening are basically slim and none. And slim left
]town.
]
]Incidentally, looking at the document (EDK II C Coding Standards Specification
]Revison 2.0), it looks like the part you quoted is section 6.4.2, not 5.4.2.
]Also, section 5.1.4.2 (General Rules) shows some examples of octal escape
]sequences commented using... C-style comments. :)
The quality of this document doesn't exactly inspire confidence
in the amount of thought that went into it. It is full of
contradictions and inconsistencies. This is reflected in the
EDK2 code base. To give some credit, both the document and
especially the code have improved over the last few years. The
C-style comment guideline is not universally followed. Ignoring
ports of existing code, where it makes sense to avoid code
changes, is not the only place. For example, /* */ is used
inside structure initializations where // won't work.
TerminalConIn.c has a couple in the C code (lines 1376, 1600).
PxeBcDhcp.c and BaseUefiDecompressLib.c too.
]> > > Just below this code in QemuFwCfgS3Enabled we ignore the returns from
]> > > QemuFwCfgSelectItem and QemuFwCfgReadBytes.
]> >
]> > Uh... QemuFwCfgSelectItem() and QemuFwCfgReadBytes() are declared as
]> > VOID. They don't return anything.
]>
]> Gah. And I thought I was able to find a counter example in 30 seconds.
]> Instead, maybe I should have been getting some sleep. :)
]>
]> I still don't think our project would come close to compiling if such
]> a warning were emitted. I would think that there are a lot more valid
]> reasons for ignoring a function return value than for setting a
]> variable value that is then not used.
]
]The EDK II project code is way better than most, owing to the fact that it
]supports more than just one C compiler. It's a great way to keep people on
]their toes (especially people who have succumbed to the scourge of GCC
]extensions).
]
]I'm not sure how many cases of unchecked return values there would actually be
]in the EDK II source (like you I thought QemuFwCfgSelectItem() and
]QemuFwCfgReadBytes() were two until I looked closer), but I suspect there are
]fewer than you might find elsewhere.
Gcc has a way to warn about this, but it requires __attribute__
((warn_unused_result))
on each function. The only easy way I could think of to add this is to
add it only to EFIAPI functions. The result shows that EDK2 has a lot
of cases similar to the classic printf example where many engineers
don't want a warning for ignored return value. With a little work,
this exclusing could be accommodated. Here are EFIAPI functions that
return a value, but the return value is ignored at least in the EDK2
code base:
AcpiTimerLibConstructor
AddUnicodeString
AddUnicodeString2
AhciDisableFisReceive
AhciEnableFisReceive
AhciStopCommand
AhciWaitMmioSet
AppendCSDGuid
AppendCSDNum
AppendCSDNum2
AppendCSDStr
AppendStringToHistory
AsciiPrint
AsciiSPrint
AsciiStrCat
AsciiStrCatS
AsciiStrCpy
AsciiStrCpyS
AsciiStrnCpy
AsciiStrnCpyS
AsciiStrToUnicodeStr
AsciiVSPrint
AsmCpuid
AsmCpuidEx
AsmMsrBitFieldWrite64
AsmWriteCr3
AsmWriteCr4
AsmWriteDr0
AsmWriteDr1
AsmWriteDr2
AsmWriteDr3
AsmWriteDr7
AsmWriteMsr64
AtaPacketReadPendingData
BasePrintLibSPrint
BcfgLibraryRegisterBcfgCommand
BcfgLibraryUnregisterBcfgCommand
BdsAddNonExistingLegacyBootOptions
BdsDeleteAllInvalidLegacyBootOptions
BdsDeleteBootOption
BdsLibBootViaBootOption
BdsLibBuildOptionFromVar
BdsLibConnectAllDefaultConsoles
BdsLibConnectAllEfi
BdsLibConnectConsoleVariable
BdsLibConnectDevicePath
BdsLibConnectUsbDevByShortFormDP
BdsLibDisconnectAllEfi
BdsLibEnumerateAllBootOption
BdsLibOutputStrings
BdsLibRegisterNewOption
BdsLibUpdateConsoleVariable
BdsRefreshBbsTableForBoot
BdsSetConsoleMode
BdsUpdateLegacyDevOrder
BltLibConfigure
BuildGuidDataHob
CascadeDelete
CatPrint
CleanUpShellParametersProtocol
CleanUpShellProtocol
Close
CloseSectionStream
CloseSimpleTextInOnFile
CloseSimpleTextOutOnFile
ConsoleLoggerDisplayHistory
ConsoleLoggerOutputStringSplit
ConsoleLoggerResetBuffers
ConsoleLoggerStopHistory
ConsoleLoggerUninstall
ConSplitterTextOutSetAttribute
ConSplitterUgaDrawSetMode
CopyGuid
CopyListOfCommandNames
CopyListOfCommandNamesWithDynamic
CopyMem
CoreCloseEvent
CoreConnectController
CoreCreateEventEx
CoreDispatcher
CoreExit
CoreFreePages
CoreFreePool
CoreInstallProtocolInterface
CoreInternalFreePool
CoreSetTimer
CoreSignalEvent
CoreUninstallProtocolInterface
CreateFullDestPath
CreateNewDeviceInfo
DebugClearMemory
DebugPortReadBuffer
DebugPortWriteBuffer
DetectAndConfigIdeDevice
DisableQuietBoot
DispatchDpc
DisplayDriverModelHandle
DivS64x64Remainder
DivU64x32Remainder
DivU64x64Remainder
DriverSampleUnload
DumpLoadedImageProtocolInfo
EbcDebugPeriodic
EbcDebugRegisterExceptionCallback
EfiAcquireLockOrFail
EfiConvertPointer
EfiCreateEventReadyToBootEx
EfiCreateProtocolNotifyEvent
EfiGetSystemConfigurationTable
EfiInitializeLock
EfiShellFreeFileList
EhcDriverBindingStart
EmuPeCoffGetThunkStucture
EnableQuietBoot
EndPerformanceMeasurement
ExecutePlatformConfig
FbGopGraphicsOutputVbeBlt
FileBufferBackup
FileBufferFree
FileBufferFreeLines
FileBufferRead
FileBufferRefresh
FileBufferRestoreMousePosition
FileBufferRestorePosition
FileBufferScrollLeft
FileBufferScrollRight
FileBufferSetFileName
FileHandleClose
FileHandleFindFirstFile
FileHandleFindNextFile
FileHandleGetPosition
FileHandleSetPosition
FreeUnicodeStringTable
FtwAbort
GetDeviceConsistMappingInfo
GetEfiGlobalVariable2
GetSmbiosStructureSize
GetVariable2
HBufferImageBufferToList
HBufferImageListToBuffer
HBufferImageRead
HiiCreateActionOpCode
HiiCreateCheckBoxOpCode
HiiCreateDateOpCode
HiiCreateDefaultOpCode
HiiCreateEndOpCode
HiiCreateGotoOpCode
HiiCreateNumericOpCode
HiiCreateOneOfOpCode
HiiCreateOneOfOptionOpCode
HiiCreateOrderedListOpCode
HiiCreateStringOpCode
HiiCreateSubTitleOpCode
HiiCreateTextOpCode
HiiCreateTimeOpCode
HiiGetBrowserData
HiiSetBrowserData
HiiSetString
HiiUpdateForm
IIO_GetCursorPosition
IIO_SetCursorPosition
InitializeListHead
InitializeSpinLock
InsertHeadList
InsertTailList
Int2OctStr
InterlockedCompareExchange32
InterlockedDecrement
InternalEfiShellSetEnv
InternalHiiAppendOpCodes
InternalHiiCreateOpCodeExtended
InternalRemoveAliasFromList
InternalShellConvertFileListType
InternalShellInitHandleList
InternalUpdateAliasOnList
Interrupt8259EndOfInterrupt
InvalidateInstructionCacheRange
IoAndThenOr16
IoBitFieldWrite16
IoOr16
IoOr8
IoWrite16
IoWrite32
IoWrite64
IoWrite8
Ip6Swap128
IpIoConfigIp
IpIoDestroy
IpIoSend
IpIoStop
KeyMapBreak
KeyMapMake
LegacyBiosInt86
LexicalInsertIntoList
LibPcdSet16
LibPcdSet32
LibPcdSet64
LibPcdSetBool
LineStrInsert
LoadDriver
MainEditorBackup
MainEditorCleanup
MainTitleBarRefresh
MicroSecondDelay
MmioAndThenOr16
MmioWrite16
MmioWrite32
MmioWrite64
MmioWrite8
MonotonicCounterDriverGetNextHighMonotonicCount
MtrrGetAllMtrrs
MtrrGetFixedMtrr
MtrrGetMemoryAttributeInVariableMtrr
MtrrGetVariableMtrr
MtrrSetAllMtrrs
MtrrSetMemoryAttribute
NanoSecondDelay
NetbufAllocSpace
NetbufBuildExt
NetbufCopy
NetbufDuplicate
NetbufGetByte
NetbufQueCopy
NetbufQueTrim
NetbufTrim
NetLibDestroyServiceChild
NetLibDetectMedia
NetLibGetMacAddress
NetListRemoveHead
NetMapInsertTail
NetMapIterate
NetMapRemoveItem
OemHookStatusCodeReport
OpenFileByDevicePath
ParseHandleDatabaseByRelationship
ParseHandleDatabaseForChildControllers
PassThruToScsiioPacket
PasswordCheck
PathCleanUpDirectories
PathRemoveLastItem
PciAnd16
PciAndThenOr32
PciCf8Write16
PciCf8Write32
PciCf8Write8
PciOr16
PciOr8
PciWrite16
PciWrite32
PciWrite8
PeCoffLoaderGetImageInfo
PeCoffLoaderUnloadImage
PeiServicesInstallPpi
PeiServicesNotifyPpi
PeiServicesReInstallPpi
PerformMappingDisplay
Pkcs7GetSigners
PreparePpisNeededByDxeCore
Print
PrintAt
PrintCharAt
PrintPciExtendedCapabilityDetails
PrintSfoVolumeInfoTableEntry
PrintStringAt
PrintStringAtWithWidth
PrintXY
ProcessCapsuleImage
QueueDpc
RandomSeed
ReleaseSpinLock
RemoveEntryList
RemoveFileTag
ReportDebugCodeEnabled
ReportErrorCodeEnabled
ReportProgressCodeEnabled
ResetPowerManagementFeature
RestoreStdInStdOutStdErr
RtMemoryStatusCodeReportWorker
RuntimeDriverCalculateCrc32
SaveAndSetDebugTimerInterrupt
SearchList
SerializeVariablesFreeInstance
SerialPortInitialize
SerialPortWrite
SerialStatusCodeReportWorker
SetDevicePathNodeLength
SetEnvironmentVariableList
SetInterruptState
SetLastError
SetMem
SetMem16
SetMem32
SetMem64
SetMemN
ShellCloseFile
ShellCloseFileMetaArg
ShellCmdDriverConfigurationProcessActionRequired
ShellCommandConsistMappingInitialize
ShellCommandConsistMappingUnInitialize
ShellCommandRegisterAlias
ShellCommandRegisterCommandName
ShellCommandSetNewScript
ShellConnectFromDevPaths
ShellConvertStringToUint64
ShellCopySearchAndReplace
ShellCreateDirectory
ShellDeleteFile
ShellFileHandleRemove
ShellGetFilePosition
ShellGetFileSize
ShellOpenFileMetaArg
ShellPrintEx
ShellPrintHiiEx
ShellPromptForResponse
ShellPromptForResponseHii
SmbiosLibCreateEntry
SmbiosLibUpdateUnicodeString
SmBusBlockProcessCall
SmBusProcessCall
SmBusReadBlock
SmBusReadDataByte
SmBusReadDataWord
SmBusReceiveByte
SmBusSendByte
SmBusWriteBlock
SmBusWriteDataByte
SmBusWriteDataWord
StartPerformanceMeasurement
StatusBarRefresh
StatusBarSetStatusString
StrCatS
StrCpy
StrCpyS
StripUnreplacedEnvironmentVariables
StrnCatGrow
StrnCatS
StrnCpy
StrnCpyS
SwapListEntries
TrimSpaces
UdpIoFreeIo
UdpIoRecvDatagram
UefiDevicePathLibCatPrint
UnicodeSPrint
UnicodeSPrintAsciiFormat
UnicodeStrToAsciiStr
UnicodeToAscii
UnicodeValueToString
UnicodeVSPrint
UnicodeVSPrintAsciiFormat
UpdateNvVarsOnFileSystem
UsbClearEndpointHalt
UsbGetProtocolRequest
UsbMassReset
UsbSetProtocolRequest
UsbSetReportRequest
ValidateAndMoveFiles
WhoAmI
WriteUnaligned16
WriteUnaligned32
WriteUnaligned64
XhcClearRootHubPortFeature
XhcControlTransfer
XhcDisableSlotCmd
XhcDisableSlotCmd64
XhcFreeEventRing
XhcPollPortStatusChange
XhcRingDoorBell
XhcSyncEventRing
XhcSyncTrsRing
ZeroMem
]It would be interesting to know if anyone
]has run the EDK II code through any static analysis tools.
]
]You might also try doing GCC builds with -ansi -pedantic -std=c99. This can
]sometimes uncover expected things, for example:
]
]In file included from /home/wpaul/edk/edk2/OvmfPkg/XenBusDxe/XenBusDxe.h:48:0,
] from /home/wpaul/edk/edk2/OvmfPkg/XenBusDxe/XenBusDxe.c:29:
]/home/wpaul/edk/edk2/OvmfPkg/Include/Protocol/XenBus.h:35:14: error: ISO C
]forbids forward references to 'enum' types [-Werror=pedantic]
] typedef enum xenbus_state XenBusState;
]
]Maybe Protocol/XenBus.h needs #include <IndustryStandard/Xen/io/xenbus.h>?
]
]Anyway, as I said before, my primary rationale for doing the change the way I
]did was that when fixing something I prefer doing it with the smallest code
]change possible, and in that case that was just sticking in a (VOID) cast. Had
]I chosen to remove the variable entirely, I probably also would have chosen to
]add the (VOID) cast to QemuFwCfgRead16(), but only because the Wind River
]coding conventions have caused me to become accustomed to doing it.
]
]-Bill
]
--
=============================================================================
-Bill Paul (510) 749-2329 | Senior Member of Technical Staff,
wp...@windriver.com | Master of Unix-Fu - Wind River Systems
------------------------------------------------------------------------------
Don't Limit Your Business. Reach for the Cloud.
GigeNET's Cloud Solutions provide you with the tools and support that
you need to offload your IT needs and focus on growing your business.
Configured For All Businesses. Start Your Cloud Today.
https://www.gigenetcloud.com/
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/edk2-devel
