1

yop,

J'aimerais récolter des critiques sur mon code, ici très simple (le binaire fait 2 ko).
Je débute, je me fais pas d'illusions. Quelles sont mes erreurs les plus grossières ? Qu'est-ce qui m'handicapera quand le programme deviendra plus gros ? Est-ce que dès éléments font que dès maintenant, on voit que ça ne sera pas maintenable ?

Il y a trois sources :
Main
#include "kernel.h"
#include "asti68k.h"
#include "config.h"
#include "funcs.h"
#include "strings.h"


// Used to quit properly
JMP_BUF JmpBuf;


/**************************************************************************************************************
*  Main function                                                                                              *
*                                                                                                             *
* Parse the command line                                                                                      *
* Create some handles for each source file                                                                    *
* Call the assembler for each file                                                                            *
* Call the linker for the whole program                                                                       *
* Wrap the final program                                                                                      *
*                                                                                                             *
**************************************************************************************************************/

void main(void)
{

// Set up exit handler
  if (setjmp(&JmpBuf))
    return;


// Clear the screen and reset point position
  clrscr();


// Create default flags mask
unsigned long Flags = (FLAG_ADDA_SUBA << BIT_ADDA_SUBA) +
                        (FLAG_LEA << BIT_LEA) +
                       (FLAG_MOVEM << BIT_MOVEM) +
                      (FLAG_QUICK << BIT_QUICK) +
                      (FLAG_XAN << BIT_XAN) +
                      (FLAG_BCC << BIT_BCC) +
                      (FLAG_STRICT << BIT_STRICT) +
                      (FLAG_ALIGN_DS << BIT_ALIGN_DS) +
                      (FLAG_ALIGN_DC << BIT_ALIGN_DC) +
                      (FLAG_ALIGN_DCB << BIT_ALIGN_DCB) +
                      (FLAG_CACHE << BIT_CACHE);


/**************************************************************************************************************
* Parse the command line                                                                                      *
*                                                                                                             *
* 0 arg : invalid, disp a message                                                                             *
* 1 arg : it must be a command, else it's invalid                                                             *
* 2 args : invalid, disp a message                                                                            *
* 3+ args : it should be a program to assemble. See Readme.txt to get informations about command line args    *
**************************************************************************************************************/

  ESI const ArgPtr = top_estack;
  unsigned short ArgCount;

  if (*ArgPtr == END_TAG)
  {
    printf(ErrorInvalidCall);
    Quit();
  }

  ArgCount = remaining_element_count(ArgPtr);

  if (ArgCount == 1)
    ReadCommand(ArgPtr, Flags);

  if (ArgCount == 2)
  {
    printf(ErrorInvalidCall);
    Quit();
  }
}


/**************************************************************************************************************
* End of main()                                                                                               *
**************************************************************************************************************/


/**************************************************************************************************************
* Quit                                                                                                        *
*                                                                                                             *
* 0 arg : disp the help                                                                                       *
**************************************************************************************************************/

void Quit(void)
{
  printf(PressAKey);
  ngetchx();
  longjmp(JmpBuf, 1);
}
Ligne de commande
#include "kernel.h"
#include "asti68k.h"
#include "funcs.h"
#include "strings.h"
#include "config.h"


/*************************************************************************
*  CheckArgType
*
*  Check if an arg pointed to by an ESI ptr is a string
* Return a ptr to the first char of the string
* Quit in case of error
*************************************************************************/

unsigned char* CheckArgType(ESI Ptr)
{
  if (*Ptr != STR_TAG)
  {
    printf(ErrorInvalidArgType);
    Quit();
  }
  Ptr -= 2;                // Skip tag & terminal \0
  while(*(Ptr--));
  return (char*) Ptr + 2;
}


/*************************************************************************
*  ReadCommand
*
*  Read an arg in the command line and execute the right routine
* Quit in case of error
*************************************************************************/

void ReadCommand(ESI Ptr, unsigned long Flags)
{
  unsigned short Count = 0;
  unsigned char* ArgPtr = CheckArgType(Ptr);
  const unsigned char* CommandPtr = StrCommands;

  while (*CommandPtr && (strcmp(CommandPtr, ArgPtr)))
  {
    Count++;
    CommandPtr += strlen(CommandPtr) + 1;
  }
  switch (Count)
  {
    case 0: printf(HelpText);
            break;

    case 1: PrintFlags(Flags);
            break;

    case 2: printf(AboutText);
            break;

    case 3: Flags = ReadConfigFile(Flags);
            PrintFlags(Flags);
            break;

    default: printf(ErrorInvalidCommand, ArgPtr);
  }
  Quit();
}


/*************************************************************************
*  PrintFlags
*
*  Disp the default flags of asTI68k
*************************************************************************/

void PrintFlags(const unsigned long Flags)
{
  const unsigned char* StrFlag = StrFlags;
  unsigned short Count = 0;
  while (*StrFlag)
  {
    printf("%s: ", StrFlag);

    if (Flags & (1 << Count))
      printf("Enabled\n");
    else
      printf("Disabled\n");

    Count++;
    StrFlag += strlen(StrFlag) + 1;
  }
  printf("cache: %s",(FLAG_CACHE ? "Enabled\n" : "Disabled\n"));
}


/*************************************************************************
*  ReadConfigFile
*
*  Read the config file and apply it to the default flags
*************************************************************************/

unsigned long ReadConfigFile(unsigned long Flags)
{
  const SYM_ENTRY* FileSym;
  const unsigned char* FilePtr;
  unsigned short Line = 1;
  PARSE_WORD ParseData;
  unsigned short ArgType;
  unsigned short FlagIndex;

  unsigned char Filename[20];
  unsigned char* NamePtr = Filename;

  *NamePtr = 0;
  NamePtr++;

// Copy default folder name if it exists
  if (*DEFAULT_PATH)
  {
    strcpy(NamePtr, DEFAULT_PATH);
    NamePtr += strlen(DEFAULT_PATH);
    *NamePtr = '\';
    NamePtr++;
  }

// Copy default file name if it exists
  if (*DEFAULT_FILE)
  {
    strcpy(NamePtr, DEFAULT_FILE);
    NamePtr += strlen(DEFAULT_FILE);
  }
  else
// else use the standard name
  {
    NamePtr = Filename + 1;
    strcpy(NamePtr, DefaultConfigFile);
    NamePtr += strlen(DefaultConfigFile);
  }

// Look for the file in the VAT
  FileSym = SymFindPtr(NamePtr, 0);
  if (!FileSym)
  {
    printf(NoConfigFileFound);
    return Flags;
  }

// Check the type of the file
  FilePtr = HeapDeref(FileSym->handle);
  if (FilePtr[*(short*)FilePtr + 1] != TEXT_TAG)
  {
    printf(ErrorConfigFileNotText);
    return Flags;
  }

  FilePtr +=2;

// Infinite loop: parse the file
// Read it and eval its args
  while (1)
  {
// Skip leading spaces, empty lines and return in case of EOF
    FilePtr = SkipSpaces(FilePtr);
    if (!*FilePtr)
      return Flags;
    if (*FilePtr == 13)
    {
      FilePtr++;
      Line++;
      continue;
    }

// Skip comments
    if (*FilePtr == '#')
    {
      FilePtr = SkipEndOfLine(FilePtr);
      continue;
    }

// Item found : it must be a global option or a flag, else skip the line
    ArgType = FLAG;
    CompareStrings(StrFlags, FilePtr, Separator1, &ParseData);
    if (!ParseData.Index)
    {
      ArgType = OPTION;
      CompareStrings(StrGlobalOptions, FilePtr, Separator1, &ParseData);
      if (!ParseData.Index)
      {
        printf(ErrorInvalidArg, Line);
        FilePtr = SkipEndOfLine(FilePtr);
        continue;
      }
    }

// Okay, we have an index, a type of table, we must find a value (O/1)
    FlagIndex = ParseData.Index;
    FilePtr += ParseData.Length;
    FilePtr = SkipSpaces(FilePtr);
    CompareStrings(StrFlagValue, FilePtr, Separator2, &ParseData);
    if (!ParseData.Index)
    {
      printf(ErrorInvalidArg, Line);
      FilePtr = SkipEndOfLine(FilePtr);
      continue;
    }

// Okay, we have a flag/option, and its value to set. Let's do it
    if (ArgType != FLAG)
      FlagIndex += 10;

    Flags = Flags | (1 << FlagIndex);
    if (ParseData.Index == 1)
      Flags = Flags & !(1 << FlagIndex);

// Now we can find spaces + [comment | EOL | EOF]. Else there is an error
    FilePtr += ParseData.Length;
    FilePtr = SkipSpaces(FilePtr);
    if ((*FilePtr == '#') || (*FilePtr == 13) || (!*FilePtr))
    {
      FilePtr = SkipEndOfLine(FilePtr);
      continue;
    }
    printf(ErrorInvalidArg, Line);
  }
}
Parseur
#include "kernel.h"
#include "funcs.h"


/************************************************
* SkipSpaces
*
* Skip the blank spaces until EOL/EOF
************************************************/

const unsigned char* SkipSpaces(const unsigned char* Ptr)
{
  while (*Ptr == 32)
    Ptr++;
  return Ptr;
}


/************************************************
* SkipEndOfLine
*
* Skip all chars of a line until EOF/EOL
************************************************/

const unsigned char* SkipEndOfLine(const unsigned char* Ptr)
{
  while (!(*Ptr || (*Ptr == 13)))
    Ptr++;
  return Ptr;
}


/************************************************
* CompareStrings
*
* Check if two strings match
* The Reference string is string list NULL terminated
* The Argument string is terminated by a char of Separator (a list of chars null-terminated)
* Return #item found in the Reference list
************************************************/

void CompareStrings(const unsigned char* Ref, const unsigned char* const Arg, const unsigned char* const Separator, PARSE_WORD* const ParseData)
{
  const unsigned char* Text;
  const unsigned char* SepItem;

  ParseData->Index = 0;

  while (*Ref)
  {
    Text = Arg;
    ParseData->Length = 0;
    (ParseData->Index)++;

  // Check if strings are identical
    while (!*Ref)
    {
      (ParseData->Length)++;
      if (*Ref++ != *Text++)
      {
        ParseData->Length = 0;
        break;
      }
    }

  // Strings mismatch. Skip current Ref, reset Arg and loop
    if (!ParseData->Length)
    {
      while (*Ref++);
      continue;
    }

    // Strings seem to match, check the Separator after Arg
    SepItem = Separator;
    while (*SepItem++)
    {
      if (*Arg == *SepItem)
        return;
    }
  }

  // Nothing match. Reset index and quit
  ParseData->Index = 0;
}


Et quelques headers :
Header principal
#ifndef __ASTI68K__
#define __ASTI68K__

/************************************************
* Bits of flags
************************************************/

#define BIT_ADDA_SUBA 0
#define BIT_LEA 1
#define BIT_MOVEM 2
#define BIT_QUICK 3
#define BIT_XAN 4
#define BIT_BCC 5
#define BIT_STRICT 6
#define BIT_ALIGN_DS 7
#define BIT_ALIGN_DC 8
#define BIT_ALIGN_DCB 9

#define BIT_CACHE 10


/************************************************
* Indicates in which table an item was found
************************************************/

#define FLAG 0
#define OPTION 1



/************************************************
* Structure used when parsing files
************************************************/

typedef struct
{
  unsigned short Length;
  unsigned short Index;
} PARSE_WORD;


#endif
Fonctions
#ifndef __FUNCS__
#define __FUNCS__

#include "asti68k.h"

extern void Quit(void);

/************************************************
* Command line
************************************************/

extern unsigned char* CheckArgType(ESI);
extern void ReadCommand(ESI const, unsigned long);
extern void PrintFlags(const unsigned long);
extern unsigned long ReadConfigFile(unsigned long);


/************************************************
* Parser
************************************************/

extern const unsigned char* SkipSpaces(const unsigned char*);
extern const unsigned char* SkipEndOfLine(const unsigned char*);
extern void CompareStrings(const unsigned char*, const unsigned char* const, const unsigned char* const, PARSE_WORD* const);
#endif[/pre]
Config
#ifndef __CONFIG__
#define __CONFIG__

#define FLAG_ADDA_SUBA 1
#define FLAG_LEA 1
#define FLAG_MOVEM 1
#define FLAG_QUICK 1
#define FLAG_XAN 1
#define FLAG_BCC 1
#define FLAG_STRICT 1
#define FLAG_ALIGN_DS 0
#define FLAG_ALIGN_DC 0
#define FLAG_ALIGN_DCB 0

#define FLAG_CACHE 0

#define DEFAULT_PATH "system"
#define DEFAULT_FILE "asti68k"

#endif

J'ai pas mis les chaines de caractères, ça me semble inutile.

Voilà, si certains ont la grande patience de m'aider, qu'ils en soient remerciés d'avance. smile

2

Pas mal, pas mal ^^

J'ai pas encore décortiqué la gestion de la ligne de commande et CompareStrings(). Tu as regardé s'il n'y avait pas des fonctions qui pouvaient t'aider
dans la bibliothèque C ? (en particulier, y'a un certain nombre de fonctions pour la gestion des chaînes, parsing, etc.)

Ce que j'ai vu pour l'instant :

void main(void)Le type de main() est normalement int (à moins qu'il y ait une particularité sur TI), pour permettre de renvoyer un code d'erreur.

// Set up exit handler    
if (setjmp(&JmpBuf))      
return;
Utilise plutôt _atexit() pour ce genre de choses smile

(FLAG_ADDA_SUBA << BIT_ADDA_SUBA)
...
Tu peux utiliser des bitfields à la place, c'est plus court et moins pénible.
Si tu tiens à le faire à la main, on combine généralement les flags avec "|" au lieu de "+"
(en temps normal c'est kif-kif, mais si tu te retrouves à activer 2 fois le même flag pour
une raison quelconque, "+" ne va pas marcher correctement ; "|" est plus sûr).

* Skip the blank spaces until EOL/EOFÇa ne correspond pas à ce que fait ta fonction (elle ne gère pas EOL/EOF)

* Skip all chars of a line until EOF/EOLLà non plus wink (ça devrait plutôt s'appeler FindEndOfLine pour être logique)
while (!(*Ptr || (*Ptr == 13)))C'est pas plutôt : while (!(!*Ptr || (*Ptr == 13)) ?
Plus lisible sous cette forme : while ((*Ptr != 0) && (*Ptr != 13))

#define FLAG_ADDA_SUBA 1
...
C'est la valeur par défaut du flag en question, non ? Tu devrais choisir un autre nom, ça prête à confusion.

#define FLAG 0 
#define OPTION 1
Une enum permet de faire la même chose sans devoir numéroter à la main.

strcpy(NamePtr, DEFAULT_PATH); 
NamePtr += strlen(DEFAULT_PATH); 
*NamePtr = '\'; 
NamePtr++; 
Tu es certain que ton buffer sera assez grand dans tous les cas ?
avatar
Zeroblog

« Tout homme porte sur l'épaule gauche un singe et, sur l'épaule droite, un perroquet. » — Jean Cocteau
« Moi je cherche plus de logique non plus. C'est surement pour cela que j'apprécie les Ataris, ils sont aussi logiques que moi ! » — GT Turbo

3

Zerosquare (./2) :
Le type de main() est normalement int (à moins qu'il y ait une particularité sur TI), pour permettre de renvoyer un code d'erreur.

Sur TI, oui c'est ça. Après, il faut un hack pour retourner une valeur.
Zerosquare (./2) :
Utilise plutôt _atexit() pour ce genre de choses smile.gif

Oui, il y a une macro dans tigcclib, un ramcall dans PreOS. On va dire que c'est un trick propre à AMS et très léger ^^
Zerosquare (./2) :
Ça ne correspond pas à ce que fait ta fonction (elle ne gère pas EOL/EOF)

Ah... euh... oui grin Entre les copier/coller pas corrigés et les bugs, faut que je revoie ça, merci grin
Zerosquare (./2) :
Là non plus wink.gif (ça devrait plutôt s'appeler FindEndOfLine pour être logique)

Tuharèzon.
Zerosquare (./2) :
C'est pas plutôt : while (!(!*Ptr || (*Ptr == 13)) ?

Merci d'avoir corriger
Zerosquare (./2) :
Plus lisible sous cette forme : while ((*Ptr != 0) && (*Ptr != 13))

J'ai toujours du mal avec les conditions quand il y en a plusieurs à la fois (pas habitué), mais t'as encore raison.
Zerosquare (./2) :
C'est la valeur par défaut du flag en question, non ? Tu devrais choisir un autre nom, ça prête à confusion.

Oui, le flag FLAG_ADDA_SUBA a la valeur 1. Ca va pas ?
Zerosquare (./2) :
Une enum permet de faire la même chose sans devoir numéroter à la main.

J'ignorais grin
Zerosquare (./2) :
Tu es certain que ton buffer sera assez grand dans tous les cas ?

Ca ne lis pas une chaine entrée par l'utilisateur en tout cas. La chaine est codée dans le binaire et n'est jamais modifiée.

4

Folco (./3) :
Oui, le flag FLAG_ADDA_SUBA a la valeur 1. Ca va pas ?
Question de goût, mais je trouve la notation FLAG_MACHIN ambiguë (ça pourrait être le numéro de bit, par exemple...). Pourquoi pas FLAG_MACHIN_DEFAULT, ou DEFAULT_MACHIN ?
Folco (./3) :
Ca ne lis pas une chaine entrée par l'utilisateur en tout cas. La chaine est codée dans le binaire et n'est jamais modifiée.
OK, pas de souci donc.
avatar
Zeroblog

« Tout homme porte sur l'épaule gauche un singe et, sur l'épaule droite, un perroquet. » — Jean Cocteau
« Moi je cherche plus de logique non plus. C'est surement pour cela que j'apprécie les Ataris, ils sont aussi logiques que moi ! » — GT Turbo

5

J'appelle BIT_ADDA_SUBA le numéro du bit.
En fait, ce header est supposé être modifiable par quelqu'un qui veut compiler le programme avec un comportement différent par défaut. Il est donc simple, que des 0/1 à changer, et ne contient que ce qui est accessible le reste du temps en ligne de commande. Remarque, ce que tu dis s'applique toujours

6

En général il me semble que les flags sont des masques à utiliser avec les opérations logiques | et &. Après, tu peux utiliser une convention différente, c’est juste un peu déroutant pour ceux qui ont l’habitude d’autre chose.
Donc pour ton cas, voici ce qui me paraîtrait le plus naturel :
// Dans asti68k.h
#define FLAG_ADDA_SUBA 1
#define FLAG_LEA 2
#define FLAG_MOVEM 4
#define FLAG_QUICK 8
…

// Dans config.h
#define DEFAULT_FLAGS (FLAG_ADDA_SUBA | FLAG_MOVEM)

// Quand tu veux tester un flag
if(flags & FLAG_MOVEM) {
  …
}

// Quand tu veux placer un flag
flags |= FLAG_ADDA_SUBA;


(PS : pourquoi mon [pre] chie ?)
avatar
« Quand le dernier arbre sera abattu, la dernière rivière empoisonnée, le dernier poisson capturé, alors vous découvrirez que l'argent ne se mange pas. »

7

C'est vrai qu'on voit souvent le type de code que tu cites, ça parait plus courant. Mais je l'ai écrit pour ça :
Folco (./5) :
En fait, ce header est supposé être modifiable par quelqu'un qui veut compiler le programme avec un comportement différent par défaut. Il est donc simple, que des 0/1 à changer

Ca évite de surcharger en commentaire pour préciser la valeur à donner si ce n'est pas 0.

8

Dans ce cas je verrais bien un truc comme ça :
#define DEFAULT_FLAG_ADDA_SUBA 1
#define DEFAULT_FLAG_LEA 1
#define DEFAULT_FLAG_MOVEM 0
…

// Create default flags mask 
unsigned long Flags = (DEFAULT_FLAG_ADDA_SUBA?FLAG_ADDA_SUBA:0) |
                        (DEFAULT_FLAG_LEA?FLAG_LEA:0) |
                        …
avatar
« Quand le dernier arbre sera abattu, la dernière rivière empoisonnée, le dernier poisson capturé, alors vous découvrirez que l'argent ne se mange pas. »

9

Bon, je suis en train de mettre en oeuvre tes différents conseils. Mais j'ai une merde avec enum :
enum chars {EOF = 0, HTAB = 9, EOL = 13, SPACE = 32};
J'ai trois fois l'erreur "Expected identifier before '(' token". Sans cette énum, ça compile à merveille. J'aime pas ce genre de truc sick

edit : Comme d'hab, faut que je poste pour trouver (vous devez trouver ça chiant, mais pour moi, ça semble tellement la recette miracle que j'hésite pas triso)

Donc avec un #undef EOF avant, ça va mieux. Mais le message d'erreur, comme toujours avec GCC, n'a rien à voir.
En effet, dans kernel.h, on a :
#define EOF -1
C'est bizare d'avoir implanté ça comme ça dans les outils de dev, ça vaut toujours 0 sur TI. Passons.

10

si tu veux des numéros de bits, à la place de
#define FLAG_ADDA_SUBA 1
#define FLAG_LEA 2
#define FLAG_MOVEM 4
#define FLAG_QUICK 8

tu peux faire

#define BIT_ADDA_SUBA 0
#define BIT_LEA 1
#define BIT_MOVEM 2
#define BIT_QUICK 3
#define FLAG_ADDA_SUBA (1<<BIT_ADDA_SUBA)
#define FLAG_LEA (1<<BIT_LEA)
#define FLAG_MOVEM (1<<BIT_MOVEM)
#define FLAG_QUICK (1<<BIT_QUICK)

11

Folco : tu peux faire des enums numérotées à la main, mais ça n'a pas beaucoup d'avantages par rapport à de simples #define.

L'intérêt, c'est surtout quand tu peux laisser le compilo choisir arbitrairement les valeurs numériques ; ça évite de dupliquer un même numéro par erreur.
Tu n'es pas obligé non plus de donner un nom à ton enum s'il n'y a pas de risque de confusion.

d'un feu tricolore :enum { vert, orange, rouge};Exemple à la con pour l'état
avatar
Zeroblog

« Tout homme porte sur l'épaule gauche un singe et, sur l'épaule droite, un perroquet. » — Jean Cocteau
« Moi je cherche plus de logique non plus. C'est surement pour cela que j'apprécie les Ataris, ils sont aussi logiques que moi ! » — GT Turbo

12

Oui. C'était un essai. grin Et ça prend moins de place que les define. Je suis encore en plein découverte du langage hein, ça doit être pour ça. grin

13

Ou:
enum FOO_BITS {BIT_ADDA_SUBA, BIT_LEA, BIT_MOVEM, BIT_QUICK};
enum FOO_FLAGS {FLAG_ADDA_SUBA = 1 << BIT_ADDA_SUBA, FLAG_LEA = 1 << BIT_LEA, FLAG_MOVEM = 1 << BIT_MOVEM, FLAG_QUICK = 1 << BIT_QUICK};

et plus rien n'est codé en dur. smile
avatar
Mes news pour calculatrices TI: Ti-Gen
Mes projets PC pour calculatrices TI: TIGCC, CalcForge (CalcForgeLP, Emu-TIGCC)
Mes chans IRC: #tigcc et #inspired sur irc.freequest.net (UTF-8)

Liberté, Égalité, Fraternité