I'm writing code for embedded systems (IAR compiler) and it should adhere to Misra C++ 2008. This limits the available features of the language quite a bit:
- no C++1x features
- use of the standard library is banned as well
- no dynamic memory (no heap)
We use PC-Lint to check for adherence.
Rule 5-0-15 of Misra states:
Array indexing is the only acceptable form of pointer arithmetic, because it is clearer and hence less error prone than pointer manipulation. This rule bans the explicit calculation of pointer values. Array indexing shall only be applied to objects defined as an array type. [...]
I have marked the sentence which my question is revolving around. I've written a small file system for our external flash memory and the file table itself resides in an EEPROM. The way to access the external EEPROM is a bit awkward. I have to derive the class from CConfigElement and I get handed a byte pointer and a data size and that array of bytes will be synchronized to the EEPROM. The complete system behind config elements and how they are working would result in probably a whole project review.
I have a known maximum number of files, I know the data size is sufficient for the number of file entries I want. My table is basically just an array of file entries. So I cast the byte pointer to a file entry pointer and work on that with array indexing.
That is clearly forbidden by the rule because the table is just a pointer and not an array. Often we are using an exception to the rule and just move on, like it is done in the code below.
A file is identified with a selector. To verify that the file stored in the file system and the requested file are actually the same, the maximum size of the file is used as a verification. The user of the code works on the external flash based on the file information provided by the file system (base address and current file size). The user also has to report back the amount of bytes written, so that the current file size can be updated.
The file system creates a new file if an unknown file is requested and aligns the base address of the new file to the 4 kiB sectors of the flash. Currently only a full reset is supported, not a deletion of single files.
We define our own types in vtypes.h which is included in all compilations:
vtypes.h
#ifndef VTypes_H
#define VTypes_H
typedef unsigned char VBOOL;
typedef char VCHAR;
typedef signed char VS8;
typedef unsigned char VU8;
typedef signed short VS16;
typedef unsigned short VU16;
typedef signed long VS32;
typedef unsigned long VU32;
typedef signed long long VS64;
typedef unsigned long long VU64;
typedef float VFLT32;
typedef double VDBL64;
static const VS8 VS8_MIN = -128;
static const VS8 VS8_MAX = 127;
static const VU8 VU8_MAX = 255;
static const VS16 VS16_MIN = -32768;
static const VS16 VS16_MAX = 32767;
static const VU16 VU16_MAX = 65535U;
static const VS32 VS32_MIN = (-2147483647L -1);
static const VS32 VS32_MAX = 2147483647L;
static const VU32 VU32_MAX = 4294967295UL;
static const VS64 VS64_MAX = 9223372036854775807LL;
static const VS64 VS64_MIN = (-9223372036854775807LL -1);
static const VU64 VU64_MAX = 18446744073709551615ULL;
static const VFLT32 VFLT32_MIN = 1.18e-38F;
static const VFLT32 VFLT32_MAX = 3.39e38F;
static const VDBL64 VDBL64_MIN = 2.23e-308;
static const VDBL64 VDBL64_MAX = 1.79e308;
#ifdef __cplusplus
#define NULL 0
#define VEXTERN_C extern "C"
#define VEXTERN_C_BEGIN extern "C" {
#define VEXTERN_C_END }
#undef TRUE
#undef FALSE
static const VBOOL TRUE = 1;
static const VBOOL FALSE = 0;
#else /* __cplusplus */
#define NULL ((void *)0)
#define TRUE 1U
#define FALSE 0U
#define VEXTERN_C
#define VEXTERN_C_BEGIN
#define VEXTERN_C_END
#endif /* __cplusplus */
#endif
Before
This is the implementation before trying to adhere to all the Misra rules.
CConfigElement.hstripped for just the essentials
#ifndef CCONFIGELEMENT_H
#define CCONFIGELEMENT_H
#include "IConfigElement.h"
class CConfigElement : public IConfigElement {
public:
virtual void SetItsData(VU8* const d, const VU32 size)
{
VASSERT(size >= dataSize);
data = d;
}
//lint -esym(1960,data,dataSize) // must be visible in derived classes
protected:
VU8* data;
VU32 dataSize;
};
#endif /* CCONFIGELEMENT_H */
CCeMiniFileSystem.h
#ifndef CCEMINIFILESYSTEM_H
#define CCEMINIFILESYSTEM_H
#include "CConfigElement.h"
class CConfig;
class CCeMiniFileSystem : public CConfigElement
{
public:
struct SoftwareImageSelector_t
{
StorageLocation_t StorageLocation;
ControllerIdentifier_t ControllerIdentifier;
ImageIdentifier_t ImageIdentifier;
};
static inline VBOOL operator==(const SoftwareImageSelector_t& lhs, const SoftwareImageSelector_t& rhs)
{
return (lhs.StorageLocation == rhs.StorageLocation) && (lhs.ControllerIdentifier == rhs.ControllerIdentifier) &&
(lhs.ImageIdentifier == rhs.ImageIdentifier);
}
static inline VBOOL operator!=(const SoftwareImageSelector_t& lhs, const SoftwareImageSelector_t& rhs)
{
return !operator==(lhs, rhs);
}
public:
CCeMiniFileSystem();
virtual ~CCeMiniFileSystem();
VBOOL GetFileInfo(SoftwareImageSelector_t fileSelector, VU32 fileMaxSize, VBOOL& isNewFile,
VU32& baseAddress, VU32& currentSize);
void UpdateCurrentSize(SoftwareImageSelector_t fileSelector, VU32 fileMaxSize,
VU32 additionalBytesWritten);
void SetConfig(CConfig* const configThisElementIsAttachedTo);
virtual void SetItsData(VU8* const d, const VU32 size);
void ResetFileSystem();
private:
VBOOL FindFile(SoftwareImageSelector_t fileSelector, VU32 fileMaxSize, VBOOL& fileExists,
VU32& fileEntryIndex);
#pragma pack(push,1)
struct FileEntry_t
{
SoftwareImageSelector_t selector;
VU32 baseAddressInFlash;
VU32 currentSizeInFlash;
VU32 MaxSize;
};
#pragma pack(pop)
const VU32 MAX_NUMBER_OF_FILES;
FileEntry_t* fileTable;
CConfig* config;
private:
CCeMiniFileSystem(const CCeMiniFileSystem&); // Only declaration -> linker error on usage
CCeMiniFileSystem& operator=(const CCeMiniFileSystem&); // Only declaration -> linker error on usage
};
#endif /* CCEMINIFILESYSTEM_H_ */
CCeMiniFileSystem.cpp
#include "CCeMiniFileSystem.h"
#include "CConfig.h"
CCeMiniFileSystem::CCeMiniFileSystem() : CConfigElement(),
MAX_NUMBER_OF_FILES(7),
fileTable(NULL),
config(NULL)
{
dataSize = MAX_NUMBER_OF_FILES * sizeof(FileEntry_t);
}
void CCeMiniFileSystem::SetItsData(VU8* const d, const VU32 size)
{
CConfigElement::SetItsData(d, size);
fileTable = reinterpret_cast<FileEntry_t*>(data);
}
CCeMiniFileSystem::~CCeMiniFileSystem()
{
}
VBOOL CCeMiniFileSystem::FindFile(SoftwareUpdate::SoftwareImageSelector_t fileSelector, VU32 fileMaxSize,
VBOOL& fileExists, VU32& fileEntryIndex)
{
fileExists = FALSE;
for (fileEntryIndex = 0U; fileEntryIndex < MAX_NUMBER_OF_FILES; fileEntryIndex++)
{
if (fileTable[fileEntryIndex].selector.StorageLocation != 2U)
{
// all files in flash have StorageLocation 2, so no files after this index
// abort here --> fileEntryIndex is index for new file
break;
}
if (fileTable[fileEntryIndex].selector == fileSelector)
{
if (fileTable[fileEntryIndex].MaxSize != fileMaxSize)
{
return FALSE; //lint !e904 file exists but has different size
}
else
{
fileExists = TRUE;
break;
}
}
}
return TRUE;
}
VBOOL CCeMiniFileSystem::GetFileInfo(SoftwareUpdate::SoftwareImageSelector_t fileSelector, VU32 fileMaxSize,
VBOOL& isNewFile, VU32& baseAddress, VU32& currentSize)
{
// Search file
VBOOL fileExists;
VU32 fileEntryIndex;
if (FALSE == FindFile(fileSelector, fileMaxSize, fileExists, fileEntryIndex))
{
return FALSE; //lint !e904 file exists but has different size
}
FileEntry_t& entryToUse = fileTable[fileEntryIndex];
if (FALSE == fileExists)
{
// new file
isNewFile = TRUE;
entryToUse.selector = fileSelector;
entryToUse.MaxSize = fileMaxSize;
entryToUse.currentSizeInFlash = 0U;
// get new baseAddress
if (fileEntryIndex == 0U)
{
// first file
entryToUse.baseAddressInFlash = 0U;
}
else
{
const VU32 indexBefore = fileEntryIndex - 1U;
const VU32 endOfFileBefore = fileTable[indexBefore].baseAddressInFlash + fileTable[indexBefore].MaxSize;
const VU32 newBaseAddress = (endOfFileBefore / 4096U + 1U) * 4096U; // align to next sector in flash
entryToUse.baseAddressInFlash = newBaseAddress;
baseAddress = newBaseAddress;
}
if (FALSE == config->Synchronize(this)) // write fileTable to EEPROM
{
return FALSE; //lint !e904 error synchronizing fileTable to EEPROM
}
}
else
{
// existing file
isNewFile = FALSE;
baseAddress = entryToUse.baseAddressInFlash;
currentSize = entryToUse.currentSizeInFlash;
}
return TRUE;
}
void CCeMiniFileSystem::UpdateCurrentSize(SoftwareUpdate::SoftwareImageSelector_t fileSelector, VU32 fileMaxSize,
VU32 additionalBytesWritten)
{
// Search file
VBOOL fileExists;
VU32 fileEntryIndex;
if (FALSE == FindFile(fileSelector, fileMaxSize, fileExists, fileEntryIndex))
{
return; //lint !e904 file exists but has different size
}
FileEntry_t& entryToUse = fileTable[fileEntryIndex];
entryToUse.currentSizeInFlash += additionalBytesWritten;
config->Synchronize(this); // write fileTable to EEPROM
}
void CCeMiniFileSystem::SetConfig(CConfig* const configThisElementIsAttachedTo)
{
config = configThisElementIsAttachedTo;
}
void CCeMiniFileSystem::ResetFileSystem()
{
for (VU32 i = 0; i < dataSize; i++)
{
data[i] = 0U;
}
config->Synchronize(this); // write fileTable to EEPROM
}
The offending lines are all those containing fileTable[fileEntryIndex] and usually in this case we would probably just add a //lint !e1960 5-0-15 comment with a justification behind it.
But I asked myself: Can I do better - can I write code that complies with the rule?
So I changed the simple pointer to FileEntry_t to a pointer to an array of known number of elements. This required to change the cast when I get the data pointer and it requires that I now dereference the pointer before I can access the element of the array. And while I was at it, I changed the loop in FindFile to have just a single break (another Misra rule).
After
CCeMiniFileSystem.h
#ifndef CCEMINIFILESYSTEM_H
#define CCEMINIFILESYSTEM_H
#include "CConfigElement.h"
class CConfig;
class CCeMiniFileSystem : public CConfigElement
{
public:
struct SoftwareImageSelector_t
{
StorageLocation_t StorageLocation;
ControllerIdentifier_t ControllerIdentifier;
ImageIdentifier_t ImageIdentifier;
};
static inline VBOOL operator==(const SoftwareImageSelector_t& lhs, const SoftwareImageSelector_t& rhs)
{
return (lhs.StorageLocation == rhs.StorageLocation) && (lhs.ControllerIdentifier == rhs.ControllerIdentifier) &&
(lhs.ImageIdentifier == rhs.ImageIdentifier);
}
static inline VBOOL operator!=(const SoftwareImageSelector_t& lhs, const SoftwareImageSelector_t& rhs)
{
return !operator==(lhs, rhs);
}
public:
CCeMiniFileSystem();
virtual ~CCeMiniFileSystem();
VBOOL GetFileInfo(SoftwareImageSelector_t fileSelector, VU32 fileMaxSize, VBOOL& isNewFile,
VU32& baseAddress, VU32& currentSize);
void UpdateCurrentSize(SoftwareImageSelector_t fileSelector, VU32 fileMaxSize,
VU32 additionalBytesWritten);
void SetConfig(CConfig* const configThisElementIsAttachedTo);
virtual void SetItsData(VU8* const d, const VU32 size);
void ResetFileSystem();
private:
VBOOL FindFile(SoftwareImageSelector_t fileSelector, VU32 fileMaxSize, VBOOL& fileExists,
VU32& fileEntryIndex);
#pragma pack(push,1)
struct FileEntry_t
{
SoftwareImageSelector_t selector;
VU32 baseAddressInFlash;
VU32 currentSizeInFlash;
VU32 MaxSize;
};
#pragma pack(pop)
static const VU32 MAX_NUMBER_OF_FILES = 7U;
FileEntry_t (* fileTable)[MAX_NUMBER_OF_FILES]; //Pointer to a FileEntry_t-array with MAX_NUMBER_OF_FILES Elements
CConfig* config;
private:
CCeMiniFileSystem(const CCeMiniFileSystem&); // Only declaration -> linker error on usage
CCeMiniFileSystem& operator=(const CCeMiniFileSystem&); // Only declaration -> linker error on usage
};
#endif /* CCEMINIFILESYSTEM_H_ */
CCeMiniFileSystem.cpp
#include "CCeMiniFileSystem.h"
#include "CConfig.h"
CCeMiniFileSystem::CCeMiniFileSystem() : CConfigElement(),
fileTable(NULL),
config(NULL)
{
dataSize = MAX_NUMBER_OF_FILES * sizeof(FileEntry_t);
}
void CCeMiniFileSystem::SetItsData(VU8* const d, const VU32 size)
{
CConfigElement::SetItsData(d, size);
fileTable = reinterpret_cast<FileEntry_t(*)[MAX_NUMBER_OF_FILES]>(data);
}
CCeMiniFileSystem::~CCeMiniFileSystem()
{
}
VBOOL CCeMiniFileSystem::FindFile(const SoftwareUpdate::SoftwareImageSelector_t fileSelector, const VU32 fileMaxSize,
VBOOL& fileExists, VU32& fileEntryIndex) const
{
fileExists = FALSE;
fileEntryIndex = 0U;
while ((fileExists == FALSE) && (fileEntryIndex < MAX_NUMBER_OF_FILES))
{
FileEntry_t& entry = (*fileTable)[fileEntryIndex];
if (entry.selector.StorageLocation != 2U)
{
// all files in flash have StorageLocation 2, so no files after this index
// abort here --> fileEntryIndex is index for new file
break;
}
if (TRUE == (entry.selector == fileSelector))
{
if (entry.MaxSize != fileMaxSize)
{
return FALSE; //lint !e904 file exists but has different size
}
else
{
fileExists = TRUE;
}
}
else
{
fileEntryIndex++; // increment only when file not found
}
}
return TRUE;
}
VBOOL CCeMiniFileSystem::GetFileInfo(const SoftwareUpdate::SoftwareImageSelector_t fileSelector, const VU32 fileMaxSize,
VBOOL& isNewFile, VU32& baseAddress,
VU32& currentSize)
{
// Search file
VBOOL fileExists;
VU32 fileEntryIndex;
if (FALSE == FindFile(fileSelector, fileMaxSize, fileExists, fileEntryIndex))
{
return FALSE; //lint !e904 file exists but has different size
}
FileEntry_t& entryToUse = (*fileTable)[fileEntryIndex];
if (FALSE == fileExists)
{
// new file
isNewFile = TRUE;
entryToUse.selector = fileSelector;
entryToUse.MaxSize = fileMaxSize;
entryToUse.currentSizeInFlash = 0U;
// determine baseAddress
if (fileEntryIndex == 0U)
{
// first file
entryToUse.baseAddressInFlash = 0U;
}
else
{
const VU32 indexBefore = fileEntryIndex - 1U;
FileEntry_t& fileBefore = (*fileTable)[indexBefore];
const VU32 endOfFileBefore = fileBefore.baseAddressInFlash + fileBefore.MaxSize;
const VU32 newBaseAddress = ((endOfFileBefore / 4096U) + 1U) * 4096U; // align to next sector in flash
entryToUse.baseAddressInFlash = newBaseAddress;
baseAddress = newBaseAddress;
}
if (FALSE == config->Synchronize(this))
{
return FALSE; //lint !e904 error synchronizing fileTable to EEPROM
}
}
else
{
// existing file
isNewFile = FALSE;
baseAddress = entryToUse.baseAddressInFlash;
currentSize = entryToUse.currentSizeInFlash;
}
return TRUE;
}
void CCeMiniFileSystem::UpdateCurrentSize(const SoftwareUpdate::SoftwareImageSelector_t fileSelector,
const VU32 fileMaxSize,
const VU32 additionalBytesWritten)
{
// Search file
VBOOL fileExists;
VU32 fileEntryIndex;
if (FALSE == FindFile(fileSelector, fileMaxSize, fileExists, fileEntryIndex))
{
return; //lint !e904 file exists but has different size
}
FileEntry_t& entryToUse = (*fileTable)[fileEntryIndex];
entryToUse.currentSizeInFlash += additionalBytesWritten;
config->Synchronize(this); //lint !e534 no possibility to use return value in a helpful way
}
void CCeMiniFileSystem::SetConfig(CConfig* const configThisElementIsAttachedTo)
{
config = configThisElementIsAttachedTo;
}
void CCeMiniFileSystem::ResetFileSystem()
{
for (VU32 i = 0U; i < dataSize; i++)
{
data[i] = 0U; //lint !e1960 5-0-15 size of array behind pointer handled with dataSize
}
config->Synchronize(this); //lint !e534 no possibility to use return value in a helpful way
}
So now I'm not really impressed with this.
The doubts I'm having:
FileEntry_t (* fileTable)[MAX_NUMBER_OF_FILES];is not going to be very well understood by everyone around me- the necessary dereferenciation makes it not easier to read or understand
So is code quality (performance is not an issue, it's readability, maintainability and being less error prone) really improved by adhering to the rule in this case?
