* [COMMIT] Improve coding standard in dbxread.c
@ 2004-11-20 12:39 Mark Kettenis
2004-11-20 17:22 ` Eli Zaretskii
0 siblings, 1 reply; 4+ messages in thread
From: Mark Kettenis @ 2004-11-20 12:39 UTC (permalink / raw)
To: gdb-patches
Only a tiny fraction of the code, but it's a step into the right
direction.
Committed,
Mark
Index: ChangeLog
from Mark Kettenis <kettenis@gnu.org>
* dbxread.c (process_one_symbol): Fix a few coding standard
issues. Improve code formatting.
Index: dbxread.c
===================================================================
RCS file: /cvs/src/src/gdb/dbxread.c,v
retrieving revision 1.77
diff -u -p -r1.77 dbxread.c
--- dbxread.c 20 Nov 2004 10:20:33 -0000 1.77
+++ dbxread.c 20 Nov 2004 12:36:59 -0000
@@ -2635,14 +2635,13 @@ read_ofile_symtab (struct partial_symtab
DESC is the desc field of the ".stab" entry.
VALU is the value field of the ".stab" entry.
NAME is the symbol name, in our address space.
- SECTION_OFFSETS is a set of amounts by which the sections of this object
- file were relocated when it was loaded into memory.
- Note that these section_offsets are not the
- objfile->section_offsets but the pst->section_offsets.
- All symbols that refer
- to memory locations need to be offset by these amounts.
- OBJFILE is the object file from which we are reading symbols.
- It is used in end_symtab. */
+ SECTION_OFFSETS is a set of amounts by which the sections of this
+ object file were relocated when it was loaded into memory. Note
+ that these section_offsets are not the objfile->section_offsets but
+ the pst->section_offsets. All symbols that refer to memory
+ locations need to be offset by these amounts.
+ OBJFILE is the object file from which we are reading symbols. It
+ is used in end_symtab. */
void
process_one_symbol (int type, int desc, CORE_ADDR valu, char *name,
@@ -2650,15 +2649,16 @@ process_one_symbol (int type, int desc,
struct objfile *objfile)
{
struct context_stack *new;
- /* This remembers the address of the start of a function. It is used
- because in Solaris 2, N_LBRAC, N_RBRAC, and N_SLINE entries are
- relative to the current function's start address. On systems
- other than Solaris 2, this just holds the SECT_OFF_TEXT value, and is
- used to relocate these symbol types rather than SECTION_OFFSETS. */
+ /* This remembers the address of the start of a function. It is
+ used because in Solaris 2, N_LBRAC, N_RBRAC, and N_SLINE entries
+ are relative to the current function's start address. On systems
+ other than Solaris 2, this just holds the SECT_OFF_TEXT value,
+ and is used to relocate these symbol types rather than
+ SECTION_OFFSETS. */
static CORE_ADDR function_start_offset;
- /* This holds the address of the start of a function, without the system
- peculiarities of function_start_offset. */
+ /* This holds the address of the start of a function, without the
+ system peculiarities of function_start_offset. */
static CORE_ADDR last_function_start;
/* If this is nonzero, we've seen an N_SLINE since the start of the
@@ -2667,8 +2667,8 @@ process_one_symbol (int type, int desc,
value is. */
static int sline_found_in_function = 1;
- /* If this is nonzero, we've seen a non-gcc N_OPT symbol for this source
- file. Used to detect the SunPRO solaris compiler. */
+ /* If this is nonzero, we've seen a non-gcc N_OPT symbol for this
+ source file. Used to detect the SunPRO solaris compiler. */
static int n_opt_found;
/* The stab type used for the definition of the last function.
@@ -2676,12 +2676,15 @@ process_one_symbol (int type, int desc,
static int function_stab_type = 0;
if (!block_address_function_relative)
- /* N_LBRAC, N_RBRAC and N_SLINE entries are not relative to the
- function start address, so just use the text offset. */
- function_start_offset = ANOFFSET (section_offsets, SECT_OFF_TEXT (objfile));
+ {
+ /* N_LBRAC, N_RBRAC and N_SLINE entries are not relative to the
+ function start address, so just use the text offset. */
+ function_start_offset =
+ ANOFFSET (section_offsets, SECT_OFF_TEXT (objfile));
+ }
- /* Something is wrong if we see real data before
- seeing a source file name. */
+ /* Something is wrong if we see real data before seeing a source
+ file name. */
if (last_source_file == NULL && type != (unsigned char) N_SO)
{
@@ -2699,8 +2702,8 @@ process_one_symbol (int type, int desc,
if (*name == '\000')
{
- /* This N_FUN marks the end of a function. This closes off the
- current block. */
+ /* This N_FUN marks the end of a function. This closes off
+ the current block. */
if (context_stack_depth <= 0)
{
@@ -2710,8 +2713,8 @@ process_one_symbol (int type, int desc,
/* The following check is added before recording line 0 at
end of function so as to handle hand-generated stabs
- which may have an N_FUN stabs at the end of the function, but
- no N_SLINE stabs. */
+ which may have an N_FUN stabs at the end of the function,
+ but no N_SLINE stabs. */
if (sline_found_in_function)
record_line (current_subfile, 0, last_function_start + valu);
@@ -2733,7 +2736,7 @@ process_one_symbol (int type, int desc,
sline_found_in_function = 0;
- /* Relocate for dynamic loading */
+ /* Relocate for dynamic loading. */
valu += ANOFFSET (section_offsets, SECT_OFF_TEXT (objfile));
valu = SMASH_TEXT_ADDRESS (valu);
last_function_start = valu;
@@ -2786,16 +2789,16 @@ process_one_symbol (int type, int desc,
lbrac_mismatch_complaint (symnum);
/* Some compilers put the variable decls inside of an
- LBRAC/RBRAC block. This macro should be nonzero if this
- is true. DESC is N_DESC from the N_RBRAC symbol.
- GCC_P is true if we've detected the GCC_COMPILED_SYMBOL
- or the GCC2_COMPILED_SYMBOL. */
+ LBRAC/RBRAC block. This macro should be nonzero if this is
+ true. DESC is N_DESC from the N_RBRAC symbol. GCC_P is true
+ if we've detected the GCC_COMPILED_SYMBOL or the
+ GCC2_COMPILED_SYMBOL. */
#if !defined (VARIABLES_INSIDE_BLOCK)
#define VARIABLES_INSIDE_BLOCK(desc, gcc_p) 0
#endif
/* Can only use new->locals as local symbols here if we're in
- gcc or on a machine that puts them before the lbrack. */
+ GCC or on a machine that puts them before the lbrack. */
if (!VARIABLES_INSIDE_BLOCK (desc, processing_gcc_compilation))
{
if (local_symbols != NULL)
@@ -2808,8 +2811,9 @@ process_one_symbol (int type, int desc,
symbols within an LBRAC/RBRAC block; this complaint
might also help sort out problems in which
VARIABLES_INSIDE_BLOCK is incorrectly defined. */
- complaint (&symfile_complaints,
- "misplaced N_LBRAC entry; discarding local symbols which have no enclosing block");
+ complaint (&symfile_complaints, "\
+misplaced N_LBRAC entry; discarding local symbols which have \
+no enclosing block");
}
local_symbols = new->locals;
}
@@ -2817,16 +2821,17 @@ process_one_symbol (int type, int desc,
if (context_stack_depth
> !VARIABLES_INSIDE_BLOCK (desc, processing_gcc_compilation))
{
- /* This is not the outermost LBRAC...RBRAC pair in the function,
- its local symbols preceded it, and are the ones just recovered
- from the context stack. Define the block for them (but don't
- bother if the block contains no symbols. Should we complain
- on blocks without symbols? I can't think of any useful purpose
- for them). */
+ /* This is not the outermost LBRAC...RBRAC pair in the
+ function, its local symbols preceded it, and are the ones
+ just recovered from the context stack. Define the block
+ for them (but don't bother if the block contains no
+ symbols. Should we complain on blocks without symbols?
+ I can't think of any useful purpose for them). */
if (local_symbols != NULL)
{
- /* Muzzle a compiler bug that makes end < start. (which
- compilers? Is this ever harmful?). */
+ /* Muzzle a compiler bug that makes end < start.
+
+ ??? Which compilers? Is this ever harmful?. */
if (new->start_addr > valu)
{
complaint (&symfile_complaints,
@@ -2854,17 +2859,16 @@ process_one_symbol (int type, int desc,
case N_FN:
case N_FN_SEQ:
- /* This kind of symbol indicates the start of an object file. */
- /* Relocate for dynamic loading */
+ /* This kind of symbol indicates the start of an object file.
+ Relocate for dynamic loading. */
valu += ANOFFSET (section_offsets, SECT_OFF_TEXT (objfile));
break;
case N_SO:
- /* This type of symbol indicates the start of data
- for one source file.
- Finish the symbol table of the previous source file
- (if any) and start accumulating a new symbol table. */
- /* Relocate for dynamic loading */
+ /* This type of symbol indicates the start of data for one
+ source file. Finish the symbol table of the previous source
+ file (if any) and start accumulating a new symbol table.
+ Relocate for dynamic loading. */
valu += ANOFFSET (section_offsets, SECT_OFF_TEXT (objfile));
n_opt_found = 0;
@@ -2872,9 +2876,9 @@ process_one_symbol (int type, int desc,
if (last_source_file)
{
/* Check if previous symbol was also an N_SO (with some
- sanity checks). If so, that one was actually the directory
- name, and the current one is the real file name.
- Patch things up. */
+ sanity checks). If so, that one was actually the
+ directory name, and the current one is the real file
+ name. Patch things up. */
if (previous_stab_code == (unsigned char) N_SO)
{
patch_subfile_names (current_subfile, name);
@@ -2884,8 +2888,8 @@ process_one_symbol (int type, int desc,
end_stabs ();
}
- /* Null name means this just marks the end of text for this .o file.
- Don't start a new symtab in this case. */
+ /* Null name means this just marks the end of text for this .o
+ file. Don't start a new symtab in this case. */
if (*name == '\000')
break;
@@ -2898,11 +2902,10 @@ process_one_symbol (int type, int desc,
break;
case N_SOL:
- /* This type of symbol indicates the start of data for
- a sub-source-file, one whose contents were copied or
- included in the compilation of the main source file
- (whose name was given in the N_SO symbol.) */
- /* Relocate for dynamic loading */
+ /* This type of symbol indicates the start of data for a
+ sub-source-file, one whose contents were copied or included
+ in the compilation of the main source file (whose name was
+ given in the N_SO symbol). Relocate for dynamic loading. */
valu += ANOFFSET (section_offsets, SECT_OFF_TEXT (objfile));
start_subfile (name, current_subfile->dirname);
break;
@@ -2922,11 +2925,12 @@ process_one_symbol (int type, int desc,
break;
case N_SLINE:
- /* This type of "symbol" really just records
- one line-number -- core-address correspondence.
- Enter it in the line list for this symbol table. */
+ /* This type of "symbol" really just records one line-number --
+ core-address correspondence. Enter it in the line list for
+ this symbol table. */
- /* Relocate for dynamic loading and for ELF acc fn-relative syms. */
+ /* Relocate for dynamic loading and for ELF acc
+ function-relative symbols. */
valu += function_start_offset;
/* GCC 2.95.3 emits the first N_SLINE stab somwehere in the
@@ -2963,28 +2967,31 @@ process_one_symbol (int type, int desc,
common_block_end (objfile);
break;
- /* The following symbol types need to have the appropriate offset added
- to their value; then we process symbol definitions in the name. */
-
- case N_STSYM: /* Static symbol in data seg */
- case N_LCSYM: /* Static symbol in BSS seg */
- case N_ROSYM: /* Static symbol in Read-only data seg */
+ /* The following symbol types need to have the appropriate
+ offset added to their value; then we process symbol
+ definitions in the name. */
+
+ case N_STSYM: /* Static symbol in data segment. */
+ case N_LCSYM: /* Static symbol in BSS segment. */
+ case N_ROSYM: /* Static symbol in read-only data segment. */
/* HORRID HACK DEPT. However, it's Sun's furgin' fault.
- Solaris2's stabs-in-elf makes *most* symbols relative
- but leaves a few absolute (at least for Solaris 2.1 and version
- 2.0.1 of the SunPRO compiler). N_STSYM and friends sit on the fence.
- .stab "foo:S...",N_STSYM is absolute (ld relocates it)
- .stab "foo:V...",N_STSYM is relative (section base subtracted).
- This leaves us no choice but to search for the 'S' or 'V'...
- (or pass the whole section_offsets stuff down ONE MORE function
- call level, which we really don't want to do). */
+ Solaris 2's stabs-in-elf makes *most* symbols relative but
+ leaves a few absolute (at least for Solaris 2.1 and version
+ 2.0.1 of the SunPRO compiler). N_STSYM and friends sit on
+ the fence. .stab "foo:S...",N_STSYM is absolute (ld
+ relocates it) .stab "foo:V...",N_STSYM is relative (section
+ base subtracted). This leaves us no choice but to search for
+ the 'S' or 'V'... (or pass the whole section_offsets stuff
+ down ONE MORE function call level, which we really don't want
+ to do). */
{
char *p;
- /* .o files and NLMs have non-zero text seg offsets, but don't need
- their static syms offset in this fashion. XXX - This is really a
- crock that should be fixed in the solib handling code so that I
- don't have to work around it here. */
+ /* Normal object file and NLMs have non-zero text seg offsets,
+ but don't need their static syms offset in this fashion.
+ XXX - This is really a crock that should be fixed in the
+ solib handling code so that I don't have to work around it
+ here. */
if (!symfile_relocatable)
{
@@ -2992,20 +2999,22 @@ process_one_symbol (int type, int desc,
if (p != 0 && p[1] == 'S')
{
/* The linker relocated it. We don't want to add an
- elfstab_offset_sections-type offset, but we *do* want
- to add whatever solib.c passed to symbol_file_add as
- addr (this is known to affect SunOS4, and I suspect ELF
- too). Since elfstab_offset_sections currently does not
- muck with the text offset (there is no Ttext.text
+ elfstab_offset_sections-type offset, but we *do*
+ want to add whatever solib.c passed to
+ symbol_file_add as addr (this is known to affect
+ SunOS 4, and I suspect ELF too). Since
+ elfstab_offset_sections currently does not muck
+ with the text offset (there is no Ttext.text
symbol), we can get addr from the text offset. If
- elfstab_offset_sections ever starts dealing with the
- text offset, and we still need to do this, we need to
- invent a SECT_OFF_ADDR_KLUDGE or something. */
+ elfstab_offset_sections ever starts dealing with
+ the text offset, and we still need to do this, we
+ need to invent a SECT_OFF_ADDR_KLUDGE or something. */
valu += ANOFFSET (section_offsets, SECT_OFF_TEXT (objfile));
goto define_a_symbol;
}
}
- /* Since it's not the kludge case, re-dispatch to the right handler. */
+ /* Since it's not the kludge case, re-dispatch to the right
+ handler. */
switch (type)
{
case N_STSYM:
@@ -3015,41 +3024,43 @@ process_one_symbol (int type, int desc,
case N_ROSYM:
goto case_N_ROSYM;
default:
- internal_error (__FILE__, __LINE__, "failed internal consistency check");
+ internal_error (__FILE__, __LINE__,
+ "failed internal consistency check");
}
}
- case_N_STSYM: /* Static symbol in data seg */
- case N_DSLINE: /* Source line number, data seg */
+ case_N_STSYM: /* Static symbol in data segment. */
+ case N_DSLINE: /* Source line number, data segment. */
valu += ANOFFSET (section_offsets, SECT_OFF_DATA (objfile));
goto define_a_symbol;
- case_N_LCSYM: /* Static symbol in BSS seg */
- case N_BSLINE: /* Source line number, bss seg */
- /* N_BROWS: overlaps with N_BSLINE */
+ case_N_LCSYM: /* Static symbol in BSS segment. */
+ case N_BSLINE: /* Source line number, BSS segment. */
+ /* N_BROWS: overlaps with N_BSLINE. */
valu += ANOFFSET (section_offsets, SECT_OFF_BSS (objfile));
goto define_a_symbol;
- case_N_ROSYM: /* Static symbol in Read-only data seg */
+ case_N_ROSYM: /* Static symbol in read-only data segment. */
valu += ANOFFSET (section_offsets, SECT_OFF_RODATA (objfile));
goto define_a_symbol;
- case N_ENTRY: /* Alternate entry point */
- /* Relocate for dynamic loading */
+ case N_ENTRY: /* Alternate entry point. */
+ /* Relocate for dynamic loading. */
valu += ANOFFSET (section_offsets, SECT_OFF_TEXT (objfile));
goto define_a_symbol;
- /* The following symbol types we don't know how to process. Handle
- them in a "default" way, but complain to people who care. */
+ /* The following symbol types we don't know how to process.
+ Handle them in a "default" way, but complain to people who
+ care. */
default:
- case N_CATCH: /* Exception handler catcher */
- case N_EHDECL: /* Exception handler name */
- case N_PC: /* Global symbol in Pascal */
- case N_M2C: /* Modula-2 compilation unit */
- /* N_MOD2: overlaps with N_EHDECL */
- case N_SCOPE: /* Modula-2 scope information */
- case N_ECOML: /* End common (local name) */
- case N_NBTEXT: /* Gould Non-Base-Register symbols??? */
+ case N_CATCH: /* Exception handler catcher. */
+ case N_EHDECL: /* Exception handler name. */
+ case N_PC: /* Global symbol in Pascal. */
+ case N_M2C: /* Modula-2 compilation unit. */
+ /* N_MOD2: overlaps with N_EHDECL. */
+ case N_SCOPE: /* Modula-2 scope information. */
+ case N_ECOML: /* End common (local name). */
+ case N_NBTEXT: /* Gould Non-Base-Register symbols??? */
case N_NBDATA:
case N_NBBSS:
case N_NBSTS:
@@ -3057,18 +3068,18 @@ process_one_symbol (int type, int desc,
unknown_symtype_complaint (hex_string (type));
/* FALLTHROUGH */
- /* The following symbol types don't need the address field relocated,
- since it is either unused, or is absolute. */
+ /* The following symbol types don't need the address field
+ relocated, since it is either unused, or is absolute. */
define_a_symbol:
- case N_GSYM: /* Global variable */
- case N_NSYMS: /* Number of symbols (ultrix) */
- case N_NOMAP: /* No map? (ultrix) */
- case N_RSYM: /* Register variable */
- case N_DEFD: /* Modula-2 GNU module dependency */
- case N_SSYM: /* Struct or union element */
- case N_LSYM: /* Local symbol in stack */
- case N_PSYM: /* Parameter variable */
- case N_LENG: /* Length of preceding symbol type */
+ case N_GSYM: /* Global variable. */
+ case N_NSYMS: /* Number of symbols (Ultrix). */
+ case N_NOMAP: /* No map? (Ultrix). */
+ case N_RSYM: /* Register variable. */
+ case N_DEFD: /* Modula-2 GNU module dependency. */
+ case N_SSYM: /* Struct or union element. */
+ case N_LSYM: /* Local symbol in stack. */
+ case N_PSYM: /* Parameter variable. */
+ case N_LENG: /* Length of preceding symbol type. */
if (name)
{
int deftype;
@@ -3085,29 +3096,31 @@ process_one_symbol (int type, int desc,
function_stab_type = type;
#ifdef SOFUN_ADDRESS_MAYBE_MISSING
- /* Deal with the SunPRO 3.0 compiler which omits the address
- from N_FUN symbols. */
+ /* Deal with the SunPRO 3.0 compiler which omits the
+ address from N_FUN symbols. */
if (type == N_FUN
- && valu == ANOFFSET (section_offsets, SECT_OFF_TEXT (objfile)))
+ && valu == ANOFFSET (section_offsets,
+ SECT_OFF_TEXT (objfile)))
{
CORE_ADDR minsym_valu =
find_stab_function_addr (name, last_source_file, objfile);
- /* find_stab_function_addr will return 0 if the minimal
- symbol wasn't found. (Unfortunately, this might also
- be a valid address.) Anyway, if it *does* return 0,
- it is likely that the value was set correctly to begin
- with... */
+ /* The function find_stab_function_addr will return
+ 0 if the minimal symbol wasn't found.
+ (Unfortunately, this might also be a valid
+ address.) Anyway, if it *does* return 0, it is
+ likely that the value was set correctly to begin
+ with... */
if (minsym_valu != 0)
valu = minsym_valu;
}
#endif
if (block_address_function_relative)
- /* For Solaris 2.0 compilers, the block addresses and
+ /* For Solaris 2 compilers, the block addresses and
N_SLINE's are relative to the start of the
- function. On normal systems, and when using gcc on
- Solaris 2.0, these addresses are just absolute, or
+ function. On normal systems, and when using GCC on
+ Solaris 2, these addresses are just absolute, or
relative to the N_SO, depending on
BLOCK_ADDRESS_ABSOLUTE. */
function_start_offset = valu;
@@ -3143,15 +3156,16 @@ process_one_symbol (int type, int desc,
/* We use N_OPT to carry the gcc2_compiled flag. Sun uses it
for a bunch of other flags, too. Someday we may parse their
flags; for now we ignore theirs and hope they'll ignore ours. */
- case N_OPT: /* Solaris 2: Compiler options */
+ case N_OPT: /* Solaris 2: Compiler options. */
if (name)
{
if (strcmp (name, GCC2_COMPILED_FLAG_SYMBOL) == 0)
{
processing_gcc_compilation = 2;
#if 0 /* Works, but is experimental. -fnf */
- /* For now, stay with AUTO_DEMANGLING for g++ output, as we don't
- know whether it will use the old style or v3 mangling. */
+ /* For now, stay with AUTO_DEMANGLING for g++ output, as
+ we don't know whether it will use the old style or v3
+ mangling. */
if (AUTO_DEMANGLING)
{
set_demangling_style (GNU_DEMANGLING_STYLE_STRING);
@@ -3177,12 +3191,12 @@ process_one_symbol (int type, int desc,
break;
/* The following symbol types can be ignored. */
- case N_OBJ: /* Solaris 2: Object file dir and name */
- case N_PATCH: /* Solaris2: Patch Run Time Checker. */
- /* N_UNDF: Solaris 2: file separator mark */
- /* N_UNDF: -- we will never encounter it, since we only process one
- file's symbols at once. */
- case N_ENDM: /* Solaris 2: End of module */
+ case N_OBJ: /* Solaris 2: Object file dir and name. */
+ case N_PATCH: /* Solaris 2: Patch Run Time Checker. */
+ /* N_UNDF: Solaris 2: File separator mark. */
+ /* N_UNDF: -- we will never encounter it, since we only process
+ one file's symbols at once. */
+ case N_ENDM: /* Solaris 2: End of module. */
case N_ALIAS: /* SunPro F77: alias name, ignore for now. */
break;
}
@@ -3194,9 +3208,9 @@ process_one_symbol (int type, int desc,
symbol. */
if (name[0] == '#')
{
- /* Initialize symbol reference names and determine if this is
- a definition. If symbol reference is being defined, go
- ahead and add it. Otherwise, just return sym. */
+ /* Initialize symbol reference names and determine if this is a
+ definition. If a symbol reference is being defined, go ahead
+ and add it. Otherwise, just return. */
char *s = name;
int refnum;
@@ -3213,7 +3227,6 @@ process_one_symbol (int type, int desc,
name = s;
}
-
previous_stab_code = type;
}
\f
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [COMMIT] Improve coding standard in dbxread.c
2004-11-20 12:39 [COMMIT] Improve coding standard in dbxread.c Mark Kettenis
@ 2004-11-20 17:22 ` Eli Zaretskii
2004-11-20 17:41 ` Mark Kettenis
0 siblings, 1 reply; 4+ messages in thread
From: Eli Zaretskii @ 2004-11-20 17:22 UTC (permalink / raw)
To: Mark Kettenis; +Cc: gdb-patches
> Date: Sat, 20 Nov 2004 13:39:09 +0100 (CET)
> From: Mark Kettenis <kettenis@gnu.org>
>
> Only a tiny fraction of the code, but it's a step into the right
> direction.
I think changes like these are wrong:
> - case N_STSYM: /* Static symbol in data seg */
> - case N_LCSYM: /* Static symbol in BSS seg */
> - case N_ROSYM: /* Static symbol in Read-only data seg */
> + case N_STSYM: /* Static symbol in data segment. */
> + case N_LCSYM: /* Static symbol in BSS segment. */
> + case N_ROSYM: /* Static symbol in read-only data segment. */
The text in these comments does not constitute a full sentence, and so
adding a period at the end is not the right change. The right way to
fix this, IMHO, is to lower-case the first letter of the comment, like
this:
case N_STSYM: /* static symbol in data segment */
There are numerous other instances of similar changes, and IMHO they
all are wrong. This one looks particularly incorrect after the
change:
> + case N_NOMAP: /* No map? (Ultrix). */
Some of the other changes simply reformat comments to break the line
at a different column. Do we have a canonical column number for that,
and if we do, what is its value?
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [COMMIT] Improve coding standard in dbxread.c
2004-11-20 17:22 ` Eli Zaretskii
@ 2004-11-20 17:41 ` Mark Kettenis
2004-11-20 18:00 ` Eli Zaretskii
0 siblings, 1 reply; 4+ messages in thread
From: Mark Kettenis @ 2004-11-20 17:41 UTC (permalink / raw)
To: eliz; +Cc: gdb-patches
Date: Sat, 20 Nov 2004 19:19:42 +0200
From: "Eli Zaretskii" <eliz@gnu.org>
> Date: Sat, 20 Nov 2004 13:39:09 +0100 (CET)
> From: Mark Kettenis <kettenis@gnu.org>
>
> Only a tiny fraction of the code, but it's a step into the right
> direction.
I think changes like these are wrong:
> - case N_STSYM: /* Static symbol in data seg */
> - case N_LCSYM: /* Static symbol in BSS seg */
> - case N_ROSYM: /* Static symbol in Read-only data seg */
> + case N_STSYM: /* Static symbol in data segment. */
> + case N_LCSYM: /* Static symbol in BSS segment. */
> + case N_ROSYM: /* Static symbol in read-only data segment. */
The text in these comments does not constitute a full sentence, and so
adding a period at the end is not the right change. The right way to
fix this, IMHO, is to lower-case the first letter of the comment, like
this:
The coding standards say:
"Also, please write complete sentences and capitalize the first word."
Now indeed it is debatable whether the comments above are complete
sentences; there's no verb in them. But these comments feel very
sentence-like. Anyway, I was aiming for some consistency here.
case N_STSYM: /* static symbol in data segment */
There are numerous other instances of similar changes, and IMHO they
all are wrong. This one looks particularly incorrect after the
change:
> + case N_NOMAP: /* No map? (Ultrix). */
Some of the other changes simply reformat comments to break the line
at a different column. Do we have a canonical column number for that,
and if we do, what is its value?
I always consider the GNU indentation style provided by emacs to be an
implementation of the canonical formatting style. The canonical
column therefore would be 32. But again, my intent here was
consistency here. The useage was very inconsistent makeing the code
difficult to read.
Mark
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [COMMIT] Improve coding standard in dbxread.c
2004-11-20 17:41 ` Mark Kettenis
@ 2004-11-20 18:00 ` Eli Zaretskii
0 siblings, 0 replies; 4+ messages in thread
From: Eli Zaretskii @ 2004-11-20 18:00 UTC (permalink / raw)
To: Mark Kettenis; +Cc: gdb-patches
> Date: Sat, 20 Nov 2004 18:41:00 +0100 (CET)
> From: Mark Kettenis <mark.kettenis@xs4all.nl>
> CC: gdb-patches@sources.redhat.com
>
> The coding standards say:
>
> "Also, please write complete sentences and capitalize the first word."
standards.texi also has this example:
#ifdef foo
...
#else /* not foo */
...
#endif /* not foo */
#ifdef foo
...
#endif /* foo */
where the comments are clearly not full sentences and don't end with a
period.
> Now indeed it is debatable whether the comments above are complete
> sentences; there's no verb in them. But these comments feel very
> sentence-like.
I'm okay with making them full sentences, if you think it's better
than what I suggested. But please let's not do half a job; let's at
least have correct English in comments. What's there now is not
correct English. Before your changes, we at least could say no one
cared to fix those comments, but now we don't have that excuse
anymore ;-).
> Anyway, I was aiming for some consistency here.
Consistency is important, but correct English is also important.
> Some of the other changes simply reformat comments to break the line
> at a different column. Do we have a canonical column number for that,
> and if we do, what is its value?
>
> I always consider the GNU indentation style provided by emacs to be an
> implementation of the canonical formatting style.
Emacs lets you customize the column where it breaks a line that is too
long. While your setup seems to use the default value, someone else
might not, or could use something other than Emacs.
So if we want to have consistent formatting, let's agree on where long
lines should be wrapped.
> The canonical column therefore would be 32.
That's where comments start; I meant the column where they are
wrapped. The default is 70 (zero-based).
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2004-11-20 18:00 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2004-11-20 12:39 [COMMIT] Improve coding standard in dbxread.c Mark Kettenis
2004-11-20 17:22 ` Eli Zaretskii
2004-11-20 17:41 ` Mark Kettenis
2004-11-20 18:00 ` Eli Zaretskii
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox