1

StackOverflow への最初の投稿 (トレード テスターであり、空き時間には非常に下手なプログラマー)。開示目的のために、これは私が苦労している大学の課題です (私が知る限り、ここで質問することは禁止されていません.

とにかく、テキスト ファイルから行を読み取り、行データをトークン化し、リンク リストを作成してから、各トークン (2 つの文字列、1 つの浮動小数点数、1 つの符号なし) をノードに挿入するプログラムがあります。ノードによって使用されているメモリが解放されるまで、すべて問題ありません。プログラム全体がクラッシュします。デバッグ後、問題を 2 つの文字列コピー操作に切り分けたようです。どちらも完全に有効に見えますが、 free() はまったく好きではありません。strncpy() を試しました - 違いはありません。文字列 char を char ごとにコピーしようとしました - 違いはありません。今、私は途方に暮れています...

誰かが見たい場合は、以下のコード (ああ、さらに開示 - ほとんど完全な C n00b です。そうです。下に悪い慣行が見られる場合、それは私です...)

#include <assert.h>
#include <stdio.h>
#include <stdlib.h>
#include "ts.h"
int main(void)
{
    /* Variables */
    FILE *stream;
    char buf[BUFSIZ];
    /* Open stock file */         //TODO - make this dynamic, don't hard-code file name
    stream = fopen("stock.csv", "r");
    assert(stream);
    /* Using addStockNode */
    while (fgets(buf, BUFSIZ, stream))
    {
        addStockNode(buf);
    }
}
void addStockNode(char* stockLine)
{
    //Create linked list
    StockNodePtr head, new, current, previous, nextStock;
    unsigned listSize;

    char *stkTok1, *stkTok2, *stkTok3, *stkTok4;
    unsigned stkLevel;
    int i;
    float stkPrice;

    listSize = 0;
    head = NULL;

        /* Create new stock node */
        if ((new = malloc(sizeof(StockNodePtr))) == NULL)
        {
            fprintf(stderr,"\nMemory allocation for node insertion failed\n");
            fprintf(stderr,"Aborting\n");
            exit(EXIT_FAILURE);
        }

        /* Tokenise data */
        stkTok1 = strtok(stockLine, ",");
        stkTok2 = strtok(NULL, ",");
        stkTok3 = strtok(NULL, ",");
        stkTok4 = strtok(NULL, ",");

        /* Search to find where in insert new list node */      //TODO - needs to be adapted to sort by stock DESCRIPTION
        current = head;
        previous = NULL;

        /* stockID */
     // strcpy(new->stockID, stkTok1);                 //falls over at free()
     // strncpy(new->stockID, stkTok1, STOCKID_LEN);   //falls over at free()

        for(i = 0; i < strlen(stkTok1); i++)           //still falls over at free()
        {
            new->stockID[i] = stkTok1[i];
        }

        /* description */
     // strcpy(new->description, stkTok2);              //falls over at free()

        /* unitPrice */
        stkPrice = strtof(stkTok3, NULL);
        new->unitPrice = stkPrice;

        /* StockLevel */
        stkLevel = strtol(stkTok4, NULL, 10);
        new->stockLevel = stkLevel;

        /*nextStock */
        new->nextStock = current;

        /* Increment listSize */
        listSize++;

        //TAKE OUT LATER - loadData can iterate through each line of the file */
        if(previous == NULL)            
        {
            head = new;
        }
        else
        {
            previous->nextStock = new;
        }

    /* Print node details */
    current = head;
    printf("%s,%s,%f,%i\n", current->stockID, current->description, current->unitPrice, current->stockLevel);       

    /* Deallocate memory used by node */
    current = head;
    while(current != NULL)
    {
        nextStock = current->nextStock;
        free(current);    //EXECUTE THIS, IT FALLS OVER (with strcpy lines uncommented)
        current = nextStock;
    }

    return EXIT_SUCCESS;
}*

完全を期すために、ここにストックノード構造体があります...

typedef struct stockNode
{
   char stockID[STOCKID_LEN + 1];
   char description[DESCRIPTION_MAX + 1];
   float unitPrice;
   unsigned stockLevel;
   StockNodePtr nextStock;
} StockNodeType;

誰かが私が間違っているところを指摘できるなら、私はそれを感謝します!

編集 - ストックノード定数は次のとおりです...

#define STOCKID_LEN 5
#define DESCRIPTION_MAX 40
#define PRICE_COLWIDTH 7
#define STOCKLEVEL_COLWIDTH 3
#define STOCKLEVEL_MAX 100

あ、それと、追加された在庫データ（別に問題ないですけど）…

S0001,Slazenger Classic Racquet,150.00,5
S0002,Slazenger Lite Racquet,98.00,3
S0003,Wilson Tournament Gold Balls,14.95,20
S0004,Dunlop Grand Prix Balls,10.95,25
S0005,Luft Nemesis Racquet,125.00,1
S0006,Wilson Tournament Balls,12.95,12
4

1 に答える 1

1

主な欠陥

あなたの根本的な問題は割り当て不足です。具体的には次のとおりです。

if ((new = malloc(sizeof(StockNodePtr))) == NULL)

これは、ポインターを保持するのに十分な大きさのスペースを割り当てていることを意味します。ストックノードではありません。

代わりにこれを試してください：

if ((new = malloc(sizeof(*new))) == NULL)

注: この記事の残りの部分を完全に読んだわけではありませんが、ポインターのサイズよりも多くのメモリをノードごとに割り当てることは決してないため、それ自体がかなり巨大です。他に問題がある場合は、私が見たときにコメントします。

総評

以下は、コード本体内の一般的な観察事項と提案であり、必ずしも主な欠陥とは関係ありません。それらのほとんどは、将来の欠陥につながる可能性のある問題を指摘しています。

変数の初期化

一般的なコーディングの実践について。初期化されていないポインターを宣言しないでください。理想的には、初期化されていないものを宣言しないでください。このようなもの：

StockNodePtr head, new, current, previous, nextStock;
unsigned listSize;

戻ってきて、本当にあなたのお尻を噛むことができます。これらは次のようになります。

StockNodePtr head=NULL, new=NULL, currentNULL, previousNULL, nextStockNULL;
unsigned listSize = 0;

宣言の直後に値を上書きするためだけに値を初期化することを心配している場合でも、心配する必要はありません。コンパイラはそれを最適化します（揮発性ではなく）。

リンクリストの構築

末尾に追加されたリンクリストを構築する際の一般的な問題は、最後の「次の」ポインターがどこにあるかを知ることです。人々はしばしば 2 つまたは 3 つのポインターでごちゃ混ぜになります。特殊なケースでは、最初の構築のヘッド ポインターなどです。

dbl-pointer (別のポインター変数のアドレスを保持するポインター) が、構築中に末尾に追加されたリストを支援するのに非常に効果的である方法を検討してください。

StockNodePtr head = NULL;
StockNodePtr *next = &head; // points to the next pointer to assign.
while(not finished)
{
    StockNodePtr newNode = malloc(sizeof(*newNode));

    // ...
    // set all your fields.
    //

    // whatever pointer `next` points to gets the new node. on an
    //  initial list it will be the `head` pointer. on a subsequent
    //  node it will be the `nextStock` pointer of the last-node-added.
    *next = newNode;

    // now just set the new next-pointer-to-populate to be the `nextStock`
    //  pointer of the node we just added.
    next = &newNode->nextStock;
}

// terminate the last node
*next = NULL;

前のコードのポインターは、次に入力するポインターのアドレスnextを常に保持します。最初に、ポインター変数のアドレスが入力されます。ループが終了したら、指しているポインタを NULL に設定してリストを終了する必要があります。注:フィールドを設定するときに設定する必要はありません。ループの次の反復で (次のノードに) 設定されるか、ループが最後に追加されたノードである場合は、ループの後に NULL に設定されます。headnextnewNode->nextStock = NULL;*next = NULL;

の使用法とそれらすべてのポインターに飛び込みstrtok()ますが、夕食に遅れて、上半身が電話しています。これが少なくともいくらか役立つことを願っています。頑張って、ダイナマイトな一日を。

于 2013-04-28T00:24:02.243 に答える