Re: gdb sources

Re: gdb sources

[André Pönitz]

[-- Attachment #1: Type: text/plain, Size: 1190 bytes --]

On Friday 12 September 2008 17:35:34 Daniel Jacobowitz wrote:
> On Fri, Sep 12, 2008 at 05:12:50PM +0200, André Pönitz wrote:
> > [...]
> > (2) Most of the "strings" in gdb are "char *", even if they are
> > conceptionally "const char *" (i.e. coming from literal, or not
> > intended to be changed). Why? In some places "const" is also
> > used, so the reason can't be "gdb supports compilers that
> > don't know about 'const'". Is it "just legacy"? If so, would patches
> > replacing "char *" by "const char *" if appropriate be welcome?
> 
> Yes, constifying patches are welcome.  It's just that gdb _used_ to
> support compilers that didn't know const, and may even predate
> const in places.

Ok. Something tiny attached for starters. It is as harmless as it can be.
I hope this is uncontroversial, but before going further into this
direction I have two related questions:

1. How would the prefered way to call, say,  xfree on a 'conceptionally const
char *' item look like? Are casts to non-const (void *) acceptable here?

2. Recording every such change in the ChangeLog basically duplicates
the work. Are there any shortcuts available/acceptable?

Andre'

[-- Attachment #2: 1 --]
[-- Type: text/x-diff, Size: 2958 bytes --]

Index: ChangeLog
===================================================================
RCS file: /cvs/src/src/gdb/ChangeLog,v
retrieving revision 1.9826
diff -r1.9826 ChangeLog
0a1,15
> 
> 2008-09-19  Andre Poenitz  <apoenitz@trolltech.com>
> 
> 	* symfile.c (allocate_symtab): Accept a const argument.
> 	* symfile.c (allocate_psymtab): Likewise.
> 	* symfile.c (free_named_symtabs): Likewise.
> 	* symfile.c (deduce_language_from_filename): Likewise.
> 
> 	* symfile.h (allocate_symtab): Update prototype.
> 	* symfile.h (allocate_psymtab): Likewise.
> 	* symfile.h (free_named_symtabs): Likewise.
> 	* symtab.h (deduce_language_from_filename): Likewise.
> 
> 	* i386-tdep.c (i386_analyze_stack_align): Fix typo in comment.
> 
Index: i386-tdep.c
===================================================================
RCS file: /cvs/src/src/gdb/i386-tdep.c,v
retrieving revision 1.266
diff -r1.266 i386-tdep.c
787c787
<   /* Rigister can't be %esp nor %ebp.  */
---
>   /* Register can't be %esp nor %ebp.  */
Index: symfile.c
===================================================================
RCS file: /cvs/src/src/gdb/symfile.c,v
retrieving revision 1.215
diff -r1.215 symfile.c
1547c1547
< 	  /* Make C the default language */
---
> 	  /* Make C the default language.  */
2688c2688
< deduce_language_from_filename (char *filename)
---
> deduce_language_from_filename (const char *filename)
2691c2691
<   char *cp;
---
>   const char *cp;
2717c2717
< allocate_symtab (char *filename, struct objfile *objfile)
---
> allocate_symtab (const char *filename, struct objfile *objfile)
2741c2741
< allocate_psymtab (char *filename, struct objfile *objfile)
---
> allocate_psymtab (const char *filename, struct objfile *objfile)
2949c2949
< free_named_symtabs (char *name)
---
> free_named_symtabs (const char *name)
3055c3055
< 		      struct section_offsets *section_offsets, char *filename,
---
> 		      struct section_offsets *section_offsets, const char *filename,
Index: symfile.h
===================================================================
RCS file: /cvs/src/src/gdb/symfile.h,v
retrieving revision 1.48
diff -r1.48 symfile.h
206c206
< extern struct symtab *allocate_symtab (char *, struct objfile *);
---
> extern struct symtab *allocate_symtab (const char *, struct objfile *);
208c208
< extern int free_named_symtabs (char *);
---
> extern int free_named_symtabs (const char *);
252c252
< 						    char *, CORE_ADDR,
---
> 						    const char *, CORE_ADDR,
302c302
< extern struct partial_symtab *allocate_psymtab (char *, struct objfile *);
---
> extern struct partial_symtab *allocate_psymtab (const char *, struct objfile *);
Index: symtab.h
===================================================================
RCS file: /cvs/src/src/gdb/symtab.h,v
retrieving revision 1.131
diff -r1.131 symtab.h
1277c1277
< extern enum language deduce_language_from_filename (char *);
---
> extern enum language deduce_language_from_filename (const char *);

Re: gdb sources

[Daniel Jacobowitz]

It looks like you don't have a copyright assignment or employer
disclaimer on file.  Is that correct?  If so, we should sort it out
now before you contribute anything larger.

On Fri, Sep 19, 2008 at 04:12:14PM +0200, André Pönitz wrote:
> Ok. Something tiny attached for starters. It is as harmless as it can be.
> I hope this is uncontroversial, but before going further into this
> direction I have two related questions:
> 
> 1. How would the prefered way to call, say,  xfree on a 'conceptionally const
> char *' item look like? Are casts to non-const (void *) acceptable
> here?

Probably acceptable, but if you know it is allocated by xfree, should
it really be returned as const?

> 2. Recording every such change in the ChangeLog basically duplicates
> the work. Are there any shortcuts available/acceptable?

If you have to update the call sites, you can say "all callers
updated" or something like that in the changelog entry, though if any
of the changes are more than mechanical then it's better to list
the complicated ones explicitly.  But for just a change to a prototype
and a definition, I think the changelog entry is still appropriate.

BTW, your changelog is too verbose in another way; there is normally
only one starred entry for a file and you can combine similar changes.
Like this:

	* symfile.c (allocate_symtab, allocate_psymtab, free_named_symtabs)
	(deduce_language_from_filename): Accept a const argument.

Please use diff -up for patches; with the default diff arguments, it's
impossible to see what you've changed.

-- 
Daniel Jacobowitz
CodeSourcery

Re: gdb sources

[André Pönitz]

[-- Attachment #1: Type: text/plain, Size: 2034 bytes --]

On Friday 19 September 2008 16:44:22 Daniel Jacobowitz wrote:
> It looks like you don't have a copyright assignment or employer
> disclaimer on file.  Is that correct?  If so, we should sort it out
> now before you contribute anything larger.

Working on that already. Might take a while, though.

The stuff in question is certainly well below any reasonable threshold
of "original art", though, but I see the problem.

> > I hope this is uncontroversial, but before going further into this
> > direction I have two related questions:
> > 
> > 1. How would the prefered way to call, say,  xfree on a 'conceptionally const
> > char *' item look like? Are casts to non-const (void *) acceptable
> > here?
> 
> Probably acceptable, but if you know it is allocated by xfree, should
> it really be returned as const?

The problem will come up with  "char *"  struct members pointing to
"names" of all sorts. They could arguably be "const char *" as they
usually aren't touched between construction and destruction to
get a compiler warning if something tries to modify them nevertheless.
But then, those members could stay non-const...

> > the work. Are there any shortcuts available/acceptable?
> 
> If you have to update the call sites, you can say "all callers
> updated" or something like that in the changelog entry, though if any
> of the changes are more than mechanical then it's better to list
> the complicated ones explicitly.  But for just a change to a prototype
> and a definition, I think the changelog entry is still appropriate.
> 
> BTW, your changelog is too verbose in another way; there is normally
> only one starred entry for a file and you can combine similar changes.
> Like this:
> 
> 	* symfile.c (allocate_symtab, allocate_psymtab, free_named_symtabs)
> 	(deduce_language_from_filename): Accept a const argument.

Ok.
 
> Please use diff -up for patches; with the default diff arguments, it's
> impossible to see what you've changed.

I intented to do that but I somehow fumbled. Next try attached.

Andre'




[-- Attachment #2: 2 --]
[-- Type: text/x-diff, Size: 5115 bytes --]

Index: ChangeLog
===================================================================
RCS file: /cvs/src/src/gdb/ChangeLog,v
retrieving revision 1.9826
diff -u -p -r1.9826 ChangeLog
--- ChangeLog	17 Sep 2008 21:48:48 -0000	1.9826
+++ ChangeLog	19 Sep 2008 15:11:01 -0000
@@ -1,3 +1,18 @@
+
+2008-09-19  Andre Poenitz  <apoenitz@trolltech.com>
+
+	* symfile.c (allocate_symtab,allocate_psymtab): Accept a const argument.
+	(allocate_psymtab): Likewise.
+	(free_named_symtabs): Likewise.
+	(deduce_language_from_filename): Likewise.
+
+	* symfile.h (allocate_symtab): Update prototype.
+	(allocate_psymtab): Likewise.
+	(free_named_symtabs): Likewise.
+	* symtab.h (deduce_language_from_filename): Likewise.
+
+	* i386-tdep.c (i386_analyze_stack_align): Fix typo in comment.
+
 2008-09-17  Jan Kratochvil  <jan.kratochvil@redhat.com>
 
 	Fix a crash on uninitialized ECS->EVENT_THREAD for a newly found thread.
Index: i386-tdep.c
===================================================================
RCS file: /cvs/src/src/gdb/i386-tdep.c,v
retrieving revision 1.266
diff -u -p -r1.266 i386-tdep.c
--- i386-tdep.c	11 Sep 2008 14:23:15 -0000	1.266
+++ i386-tdep.c	19 Sep 2008 15:11:15 -0000
@@ -784,7 +784,7 @@ i386_analyze_stack_align (CORE_ADDR pc, 
       offset = 5;
     }
 
-  /* Rigister can't be %esp nor %ebp.  */
+  /* Register can't be %esp nor %ebp.  */
   if (reg == 4 || reg == 5)
     return pc;
 
Index: symfile.c
===================================================================
RCS file: /cvs/src/src/gdb/symfile.c,v
retrieving revision 1.215
diff -u -p -r1.215 symfile.c
--- symfile.c	13 Sep 2008 17:28:56 -0000	1.215
+++ symfile.c	19 Sep 2008 15:11:30 -0000
@@ -1544,7 +1544,7 @@ set_initial_language (void)
 
       if (lang == language_unknown)
 	{
-	  /* Make C the default language */
+	  /* Make C the default language.  */
 	  lang = language_c;
 	}
 
@@ -2685,10 +2685,10 @@ init_filename_language_table (void)
 }
 
 enum language
-deduce_language_from_filename (char *filename)
+deduce_language_from_filename (const char *filename)
 {
   int i;
-  char *cp;
+  const char *cp;
 
   if (filename != NULL)
     if ((cp = strrchr (filename, '.')) != NULL)
@@ -2714,7 +2714,7 @@ deduce_language_from_filename (char *fil
  */
 
 struct symtab *
-allocate_symtab (char *filename, struct objfile *objfile)
+allocate_symtab (const char *filename, struct objfile *objfile)
 {
   struct symtab *symtab;
 
@@ -2738,7 +2738,7 @@ allocate_symtab (char *filename, struct 
 }
 
 struct partial_symtab *
-allocate_psymtab (char *filename, struct objfile *objfile)
+allocate_psymtab (const char *filename, struct objfile *objfile)
 {
   struct partial_symtab *psymtab;
 
@@ -2946,7 +2946,7 @@ cashier_psymtab (struct partial_symtab *
    all stray pointers into the freed symtab.  */
 
 int
-free_named_symtabs (char *name)
+free_named_symtabs (const char *name)
 {
 #if 0
   /* FIXME:  With the new method of each objfile having it's own
@@ -3052,7 +3052,7 @@ again2:
 
 struct partial_symtab *
 start_psymtab_common (struct objfile *objfile,
-		      struct section_offsets *section_offsets, char *filename,
+		      struct section_offsets *section_offsets, const char *filename,
 		      CORE_ADDR textlow, struct partial_symbol **global_syms,
 		      struct partial_symbol **static_syms)
 {
Index: symfile.h
===================================================================
RCS file: /cvs/src/src/gdb/symfile.h,v
retrieving revision 1.48
diff -u -p -r1.48 symfile.h
--- symfile.h	5 Sep 2008 11:37:17 -0000	1.48
+++ symfile.h	19 Sep 2008 15:11:30 -0000
@@ -203,9 +203,9 @@ extern void init_psymbol_list (struct ob
 
 extern void sort_pst_symbols (struct partial_symtab *);
 
-extern struct symtab *allocate_symtab (char *, struct objfile *);
+extern struct symtab *allocate_symtab (const char *, struct objfile *);
 
-extern int free_named_symtabs (char *);
+extern int free_named_symtabs (const char *);
 
 extern void add_symtab_fns (struct sym_fns *);
 
@@ -249,7 +249,7 @@ extern void free_section_addr_info (stru
 
 extern struct partial_symtab *start_psymtab_common (struct objfile *,
 						    struct section_offsets *,
-						    char *, CORE_ADDR,
+						    const char *, CORE_ADDR,
 						    struct partial_symbol **,
 						    struct partial_symbol **);
 
@@ -299,7 +299,7 @@ extern int auto_solib_limit;
 
 extern void set_initial_language (void);
 
-extern struct partial_symtab *allocate_psymtab (char *, struct objfile *);
+extern struct partial_symtab *allocate_psymtab (const char *, struct objfile *);
 
 extern void discard_psymtab (struct partial_symtab *);
 
Index: symtab.h
===================================================================
RCS file: /cvs/src/src/gdb/symtab.h,v
retrieving revision 1.131
diff -u -p -r1.131 symtab.h
--- symtab.h	5 Sep 2008 11:37:17 -0000	1.131
+++ symtab.h	19 Sep 2008 15:11:30 -0000
@@ -1274,7 +1274,7 @@ extern struct symtab_and_line find_funct
 
 extern void clear_symtab_users (void);
 
-extern enum language deduce_language_from_filename (char *);
+extern enum language deduce_language_from_filename (const char *);
 
 /* symtab.c */
 

Re: gdb sources

[Daniel Jacobowitz]

On Fri, Sep 19, 2008 at 05:20:13PM +0200, André Pönitz wrote:
> > Please use diff -up for patches; with the default diff arguments, it's
> > impossible to see what you've changed.
> 
> I intented to do that but I somehow fumbled. Next try attached.

Sorry, one more issue I didn't mention - please keep changelog entries
as plain text, not a patch.  They always conflict.

> @@ -1,3 +1,18 @@
> +
> +2008-09-19  Andre Poenitz  <apoenitz@trolltech.com>
> +
> +	* symfile.c (allocate_symtab,allocate_psymtab): Accept a const argument.
> +	(allocate_psymtab): Likewise.
> +	(free_named_symtabs): Likewise.
> +	(deduce_language_from_filename): Likewise.
> +
> +	* symfile.h (allocate_symtab): Update prototype.
> +	(allocate_psymtab): Likewise.
> +	(free_named_symtabs): Likewise.
> +	* symtab.h (deduce_language_from_filename): Likewise.

It looks like you started rewriting this entry but didn't finish?

-- 
Daniel Jacobowitz
CodeSourcery

Re: gdb sources

[André Pönitz]

On Friday 19 September 2008 17:25:50 Daniel Jacobowitz wrote:
> On Fri, Sep 19, 2008 at 05:20:13PM +0200, André Pönitz wrote:
> > > Please use diff -up for patches; with the default diff arguments, it's
> > > impossible to see what you've changed.
> > 
> > I intented to do that but I somehow fumbled. Next try attached.
> 
> Sorry, one more issue I didn't mention - please keep changelog entries
> as plain text, not a patch.  They always conflict.
> 
> > @@ -1,3 +1,18 @@
> > +
> > +2008-09-19  Andre Poenitz  <apoenitz@trolltech.com>
> > +
> > +	* symfile.c (allocate_symtab,allocate_psymtab): Accept a const argument.
> > +	(allocate_psymtab): Likewise.
> > +	(free_named_symtabs): Likewise.
> > +	(deduce_language_from_filename): Likewise.
> > +
> > +	* symfile.h (allocate_symtab): Update prototype.
> > +	(allocate_psymtab): Likewise.
> > +	(free_named_symtabs): Likewise.
> > +	* symtab.h (deduce_language_from_filename): Likewise.
> 
> It looks like you started rewriting this entry but didn't finish?

Not really. It's symfile.h in the beginning, and symtab.h in the end. 

There's precedence for having no empty lines in the file contents 
below, also for mixed (with and without empty lines) style within a
single Changelog entry, so I figured having no line might be 
prefered if the stuff belongs close together (as in "Likewise,") and 
an empty line belong where the differences are bigger (if they are
bigger in this case...)

Incidentally, sticking to rules would be much easier if there were
a blurb on the do's or don't's somewhere.

Andre'

Re: gdb sources

[Daniel Jacobowitz]

On Fri, Sep 19, 2008 at 05:45:40PM +0200, André Pönitz wrote:
> > > +2008-09-19  Andre Poenitz  <apoenitz@trolltech.com>
> > > +
> > > +	* symfile.c (allocate_symtab,allocate_psymtab): Accept a const argument.
> > > +	(allocate_psymtab): Likewise.
> > > +	(free_named_symtabs): Likewise.
> > > +	(deduce_language_from_filename): Likewise.
> > > +
> > > +	* symfile.h (allocate_symtab): Update prototype.
> > > +	(allocate_psymtab): Likewise.
> > > +	(free_named_symtabs): Likewise.
> > > +	* symtab.h (deduce_language_from_filename): Likewise.
> > 
> > It looks like you started rewriting this entry but didn't finish?
> 
> Not really. It's symfile.h in the beginning, and symtab.h in the end. 

Here's what I meant:

2008-09-19  Andre Poenitz  <apoenitz@trolltech.com>

	* symfile.c (allocate_symtab, allocate_psymtab, free_named_symtabs)
	(deduce_language_from_filename): Accept a const argument.
	* symfile.h (allocate_symtab, allocate_psymtab)
	(free_named_symtabs): Update prototypes.
	* symtab.h (deduce_language_from_filename): Likewise.

> There's precedence for having no empty lines in the file contents 
> below, also for mixed (with and without empty lines) style within a
> single Changelog entry, so I figured having no line might be 
> prefered if the stuff belongs close together (as in "Likewise,") and 
> an empty line belong where the differences are bigger (if they are
> bigger in this case...)

Right - I wouldn't personally put the empty lines where you did, but
there's nothing wrong with it either.

> Incidentally, sticking to rules would be much easier if there were
> a blurb on the do's or don't's somewhere.

In most cases - including this one - we defer to the GNU Coding
Standards document which is available on gnu.org.

-- 
Daniel Jacobowitz
CodeSourcery