patterncMinor
Implementation of C linked list (Queue) of C strings on mega AVR
Viewed 0 times
avrstringsmegaimplementationlistlinkedqueue
Problem
I have been working on a mechanism to queue up strings coming into a system via a USART and have implemented it as a singly linked list. I have done my best to profile the code and its memory usage at runtime depending on the amount of data fed to the data structure, but can anyone see anything "wrong" or less than desirable in the code?
StringQueue.h
StringQueue.c
```
/**
* @author Luke R Gary
* @date 6/1/2015
*
* @note Not debugged, prolly leakier than a diarrhea butt
*/
#include "StringQueue.h"
/**
* @brief returns string at the front of the queue
* @details [long description]
*
* @param queue a queue structure reference
* @return pointer to string
*/
char queueFront(strqueue_t queue)
{
if ((queue->front != NULL) && (queue->rear != NULL))
return (queue->front->data);
else
return 0;
}
/**
* @brief [brief description]
* @details [long description]
*
* @param queue a queue structure reference
* @param str [description]
*
* @return [description]
*/
uint8_t enqueue(strqueue_t queue, const char str)
{
// if first element and last element in queue. i.e. no allocations yet
if(queue->front == NULL)
{
qu
StringQueue.h
/**
* @author Luke R Gary
* @date 6/1/2015
*
* @note Not debugged, prolly leakier than a diarrhea butt
*
* @note Look for a way to implement this driver to store strings in flash memory at the expense of
* allocation/traverse time
*/
#ifndef StringQueue_h
#define StringQueue_h
#include "stdlib.h"
#include "stdint.h"
#include "string.h"
#include "stdio.h"
#define MAX_ELEMENTS 32
//!data;
free(str);
}
///
extern char * dequeue(strqueue_t * queue);
///
extern uint8_t queueIsEmpty(const strqueue_t * queue);
///
extern void queueDisplay(strqueue_t * queue);
///
extern void queueCreate(strqueue_t * queue, uint8_t elements);
///
extern uint8_t queueSize(const strqueue_t * queue);
///
extern uint8_t queueIsFull(const strqueue_t * queue);
#endifStringQueue.c
```
/**
* @author Luke R Gary
* @date 6/1/2015
*
* @note Not debugged, prolly leakier than a diarrhea butt
*/
#include "StringQueue.h"
/**
* @brief returns string at the front of the queue
* @details [long description]
*
* @param queue a queue structure reference
* @return pointer to string
*/
char queueFront(strqueue_t queue)
{
if ((queue->front != NULL) && (queue->rear != NULL))
return (queue->front->data);
else
return 0;
}
/**
* @brief [brief description]
* @details [long description]
*
* @param queue a queue structure reference
* @param str [description]
*
* @return [description]
*/
uint8_t enqueue(strqueue_t queue, const char str)
{
// if first element and last element in queue. i.e. no allocations yet
if(queue->front == NULL)
{
qu
Solution
Standard headers should be included between `
As a side note, since you haven't edited your doxygen comment blocks, maybe just remove them. As they stand, it only serves to clutter your source files.
. By using quotes, your are telling the compiler to look first into the current directory, then in the standard include path. This is a waste of time, since I doubt that you have a custom C library with your project?
#include
#include
#include
#include
Unless you plan on having your code compile as C++, then you should avoid casting the return value of malloc. In C void* converts implicitly to any other pointer type. This is not true for C++.
Unless you need to compile in dual mode, it is best to avoid the cast, since it introduces maintenance overhead and some repetition of types.
__inline__ void removeData(node_t * node)
__inline__ (with the double underscores) is a GCC extension from the time inline wasn't standard yet. Unless your compiler doesn't support inline, you have no reason to use the GCC specific keyword.
The extern on your function prototypes is not necessary. It is allowed on function prototypes to be orthogonal with global variables, I'm guessing. Removing it should have no side effects. Since it only adds verbosity to the header file, I don't see any reason to use it on function prototypes.
Bit of a personal preference here, but in a place such as:
uint8_t queueIsFull(const strqueue_t *queue)
{
if(queue->front != NULL && queue->nodeCount >= MAX_ELEMENTS)
return 1;
else
return 0;
}
I would avoid the if/else and simply return the expression:
uint8_t queueIsFull(const strqueue_t *queue)
{
return queue->front != NULL && queue->nodeCount >= MAX_ELEMENTS;
}
By the way, that function returns true or false, so its return type should be bool. (Note: include somewhere).
Arguably another piece of personal preference, but I think the following is making an excessive use of parenthesis:
char * queueFront(strqueue_t * queue)
{
if ((queue->front != NULL) && (queue->rear != NULL))
return (queue->front->data);
else
return 0;
}
The ( ) pair in the return statement is useless. return is not a function call. In the if statement they also make no difference. And you are not consistently using the style of wrapping each part of an if in ( ), so I would just drop it.
This is a very minor point, but include guards, being preprocessor directives, are usually defined using the ALL_UPPERCASE notation:
#ifndef STRING_QUEUE_H
#define STRING_QUEUE_H
I personally recommend always adding curly braces { }` on all if/else statements. It will make your code more maintainable and might even help avoiding bugs like the (in)famous goto fail from OpenSSL.As a side note, since you haven't edited your doxygen comment blocks, maybe just remove them. As they stand, it only serves to clutter your source files.
Code Snippets
#include <stdlib.h>
#include <stdint.h>
#include <string.h>
#include <stdio.h>__inline__ void removeData(node_t * node)uint8_t queueIsFull(const strqueue_t *queue)
{
if(queue->front != NULL && queue->nodeCount >= MAX_ELEMENTS)
return 1;
else
return 0;
}uint8_t queueIsFull(const strqueue_t *queue)
{
return queue->front != NULL && queue->nodeCount >= MAX_ELEMENTS;
}char * queueFront(strqueue_t * queue)
{
if ((queue->front != NULL) && (queue->rear != NULL))
return (queue->front->data);
else
return 0;
}Context
StackExchange Code Review Q#94721, answer score: 6
Revisions (0)
No revisions yet.