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