[C++/testsuite] Remove pchtest check objects and compile with current tool
diff mbox

Message ID 20131030095618.GB22154@nbbrfq.cc.univie.ac.at
State New
Headers show

Commit Message

Bernhard Reutner-Fischer Oct. 30, 2013, 9:56 a.m. UTC
Hi,

The pch-init leaves check objects lying around, remove them.

While at it, i noticed that i was getting warnings from the check since
it was invoced with xg++ -nostdinc++ on C source (in one of the two
iterations the check is run -- once per tool).
My suggestion is to use the correct tool to perform the pch check. Does
that make sense to you?

Regstrapped (together with the auto-removal patch just sent) on
x86_64-unknown-linux-gnu with trunk@204119 without regressions.

Ok for trunk?

gcc/testsuite/ChangeLog

2013-10-12  Bernhard Reutner-Fischer  <aldot@gcc.gnu.org>

	* lib/dg-pch.exp (pch-init): Remove pchtest check objects.
	Compile pchtest with current tool.

Comments

Mike Stump Oct. 30, 2013, 9:47 p.m. UTC | #1
On Oct 30, 2013, at 2:56 AM, Bernhard Reutner-Fischer <rep.dot.nop@gmail.com> wrote:
> -	set result [check_compile pchtest object "int i;" "-x c-header"]
> +	set result [check_compile pchtest object "$chk_type" "$chk_lang"]

the patch uses chk_type, but, I can't find where it is being set?

Was there a significant purpose for the added C++ comment?  If not, can you remove that?  If so, can you explain?

Last question I have is the remove-build-file primitive.  I'm wondering on a canadian cross, are the files left over on the build machine, the host machine or both the build machine and the host machine?
I see people use remote_file build delete …, file_on_host delete and remove-build-file.  Some folks even use the plain file delete.  I'd hate to guess which one you need, it hurts my brain.  I think remove-build-file is safe; just don't know if it is best.  Anyone else want to weigh in?
Bernhard Reutner-Fischer Oct. 30, 2013, 10:14 p.m. UTC | #2
On 30 October 2013 22:47, Mike Stump <mikestump@comcast.net> wrote:
> On Oct 30, 2013, at 2:56 AM, Bernhard Reutner-Fischer <rep.dot.nop@gmail.com> wrote:
>> -     set result [check_compile pchtest object "int i;" "-x c-header"]
>> +     set result [check_compile pchtest object "$chk_type" "$chk_lang"]
>
> the patch uses chk_type, but, I can't find where it is being set?

hmz yea, that should read $chk_content
>
> Was there a significant purpose for the added C++ comment?  If not, can you remove that?  If so, can you explain?

grep -A9 "CONTENTS is" gcc/testsuite/lib/target-supports.exp
# Assume by default that CONTENTS is C code.
# Otherwise, code should contain:
# "// C++" for c++,
# "! Fortran" for Fortran code,
# "/* ObjC", for ObjC
# "// ObjC++" for ObjC++
# and "// Go" for Go
# If the tool is ObjC/ObjC++ then we overide the extension to .m/.mm to
# allow for ObjC/ObjC++ specific flags.
proc check_compile {basename type contents args} {
>
> Last question I have is the remove-build-file primitive.  I'm wondering on a canadian cross, are the files left over on the build machine, the host machine or both the build machine and the host machine?

I don't really remember, i didn't run canadian cross tests on remote
boxes since ages, TBH.

> I see people use remote_file build delete …, file_on_host delete and remove-build-file.  Some folks even use the plain file delete.  I'd hate to guess which one you need, it hurts my brain.  I think remove-build-file is safe; just don't know if it is best.

remove-build-file certainly wipes it from everywhere so seems the safe bet.
But yes, for this specific pchtest.o's one could refine the delete to
the appropriate build or host. I would think that using plain delete
is wrong everywhere though.

> Anyone else want to weigh in?
Mike Stump Oct. 30, 2013, 10:22 p.m. UTC | #3
On Oct 30, 2013, at 3:14 PM, Bernhard Reutner-Fischer <rep.dot.nop@gmail.com> wrote:
> On 30 October 2013 22:47, Mike Stump <mikestump@comcast.net> wrote:
>> 
>> Was there a significant purpose for the added C++ comment?  If not, can you remove that?  If so, can you explain?
> 
> grep -A9 "CONTENTS is" gcc/testsuite/lib/target-supports.exp
> # Assume by default that CONTENTS is C code.
> # Otherwise, code should contain:
> # "// C++" for c++,
> # "! Fortran" for Fortran code,
> # "/* ObjC", for ObjC
> # "// ObjC++" for ObjC++
> # and "// Go" for Go
> # If the tool is ObjC/ObjC++ then we overide the extension to .m/.mm to
> # allow for ObjC/ObjC++ specific flags.
> proc check_compile {basename type contents args} {

Ah, but this is why I asked for a significant purpose?  The language of the file selects the options (flags) allowed.  The language is set in your code.  I think it was part of trying different ways to fix it, but, it turned out to be neither necessary or sufficient in the end.
Bernhard Reutner-Fischer Oct. 31, 2013, 8:47 a.m. UTC | #4
On 30 October 2013 23:22, Mike Stump <mikestump@comcast.net> wrote:
> On Oct 30, 2013, at 3:14 PM, Bernhard Reutner-Fischer <rep.dot.nop@gmail.com> wrote:
>> On 30 October 2013 22:47, Mike Stump <mikestump@comcast.net> wrote:
>>>
>>> Was there a significant purpose for the added C++ comment?  If not, can you remove that?  If so, can you explain?
>>
>> grep -A9 "CONTENTS is" gcc/testsuite/lib/target-supports.exp
>> # Assume by default that CONTENTS is C code.
>> # Otherwise, code should contain:
>> # "// C++" for c++,
>> # "! Fortran" for Fortran code,
>> # "/* ObjC", for ObjC
>> # "// ObjC++" for ObjC++
>> # and "// Go" for Go
>> # If the tool is ObjC/ObjC++ then we overide the extension to .m/.mm to
>> # allow for ObjC/ObjC++ specific flags.
>> proc check_compile {basename type contents args} {
>
> Ah, but this is why I asked for a significant purpose?  The language of the file selects the options (flags) allowed.  The language is set in your code.  I think it was part of trying different ways to fix it, but, it turned out to be neither necessary or sufficient in the end.

Not sure about any significant purpose, no.
I found it odd that the check did not attempt to obtain a result
without knowingly provoking an odd warning, hence these chk_ stuff.
So, what do you want me to do? I want to delete the test objects and i
don't really care if remove-build-file overdoes it or not..
pch usually fails for my crosses anyway so is disabled in the first place :P

Are you saying that these CONTENT stuff should be nuked altogether
and/or the pchtest stanza be kept as is and/or the stanza be cut to
only fire for tool==g++ ?

thanks,
Mike Stump Nov. 1, 2013, 1:15 a.m. UTC | #5
On Oct 31, 2013, at 1:47 AM, Bernhard Reutner-Fischer <rep.dot.nop@gmail.com> wrote:
> On 30 October 2013 23:22, Mike Stump <mikestump@comcast.net> wrote:
>> On Oct 30, 2013, at 3:14 PM, Bernhard Reutner-Fischer <rep.dot.nop@gmail.com> wrote:
>>> On 30 October 2013 22:47, Mike Stump <mikestump@comcast.net> wrote:
>>>> 
>>>> Was there a significant purpose for the added C++ comment?  If not, can you remove that?  If so, can you explain?
>>> 
>>> grep -A9 "CONTENTS is" gcc/testsuite/lib/target-supports.exp
>>> # Assume by default that CONTENTS is C code.
>>> # Otherwise, code should contain:
>>> # "// C++" for c++,
>>> # "! Fortran" for Fortran code,
>>> # "/* ObjC", for ObjC
>>> # "// ObjC++" for ObjC++
>>> # and "// Go" for Go
>>> # If the tool is ObjC/ObjC++ then we overide the extension to .m/.mm to
>>> # allow for ObjC/ObjC++ specific flags.
>>> proc check_compile {basename type contents args} {
>> 
>> Ah, but this is why I asked for a significant purpose?  The language of the file selects the options (flags) allowed.  The language is set in your code.  I think it was part of trying different ways to fix it, but, it turned out to be neither necessary or sufficient in the end.
> 
> Not sure about any significant purpose, no.

Ok, then it can be safely removed.

> So, what do you want me to do?

Remove the added comment…   and repost…

Thanks.

Patch
diff mbox

diff --git a/gcc/testsuite/lib/dg-pch.exp b/gcc/testsuite/lib/dg-pch.exp
index d82c669..41de454 100644
--- a/gcc/testsuite/lib/dg-pch.exp
+++ b/gcc/testsuite/lib/dg-pch.exp
@@ -18,6 +18,18 @@  load_lib copy-file.exp
 
 proc pch-init { args } {
     global pch_unsupported_debug pch_unsupported
+    global tool
+
+    set chk_content ""
+    set chk_lang "c-header"
+    switch -- "$tool" {
+	"g++" {
+		set chk_content "// C++"
+		set chk_lang "c++-header"
+	      }
+    }
+    append chk_content "\nint i;"
+    set chk_lang "-x $chk_lang"
 
     if [info exists pch_unsupported_debug] {
 	error "pch-init: pch_unsupported_debug is not empty as expected"
@@ -26,20 +38,22 @@  proc pch-init { args } {
 	error "pch-init: pch_unsupported is not empty as expected"
     }
 
-    set result [check_compile pchtest object "int i;" "-g -x c-header"]
+    set result [check_compile pchtest object "$chk_content" "-g $chk_lang"]
     set pch_unsupported_debug \
 	[regexp "debug format cannot be used with pre-compiled headers" \
 		[lindex $result 0]]
+    remove-build-file [lindex $result 1]
 
     set pch_unsupported 0
     if { $pch_unsupported_debug } {
 	verbose -log "pch is unsupported with the debug info format"
 
-	set result [check_compile pchtest object "int i;" "-x c-header"]
+	set result [check_compile pchtest object "$chk_type" "$chk_lang"]
 	    set pch_unsupported \
 		[regexp "debug format cannot be used with pre-compiled headers" \
 			[lindex $result 0]]
     }
+    remove-build-file [lindex $result 1]
 }
 
 proc pch-finish { args } {