From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 11905 invoked by alias); 18 May 2017 11:34:06 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Received: (qmail 11299 invoked by uid 89); 18 May 2017 11:34:05 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-10.3 required=5.0 tests=AWL,BAYES_00,GIT_PATCH_2,GIT_PATCH_3,KAM_LAZY_DOMAIN_SECURITY,RCVD_IN_DNSWL_NONE,RCVD_IN_SORBS_SPAM autolearn=ham version=3.3.2 spammy= X-HELO: mail-wr0-f169.google.com Received: from mail-wr0-f169.google.com (HELO mail-wr0-f169.google.com) (209.85.128.169) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Thu, 18 May 2017 11:34:03 +0000 Received: by mail-wr0-f169.google.com with SMTP id z52so31652041wrc.2 for ; Thu, 18 May 2017 04:34:06 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:subject:to:references:cc:from:message-id:date :user-agent:mime-version:in-reply-to:content-transfer-encoding; bh=cI1HgPbkfC5jNMN22+EWVprKscxGLqDtsk3U72/2pRI=; b=FTpT4d4ml18+dp+r7rgIgBpXW0xilk15vLmx48HOoA0vNFZWo2cr0HwLfSwoD78hrA ERA8D0QAGiOzwlzVv/MexRnyWpYXuXSQzyyu/qUjDHcsIodgWK3nKQ8F/R/55m/nbeA2 jiXhR5+/e1g2nGDYem/Hpk7qgHNDJcVIJnkixycavCgh1XZEOyH8I2i3XWUje6+OhfHi q4/QOGkXzan0RuMnThAEm98CdXRmbFs7IbSxlOsuK0to7T9FIgbOpvAQ5pYzcyTWwFFZ txva1NhhncTUY1xkFITptM2wtMYC4yXc/HyU75q+O2UrXPmlRK1aVQTj25BC5GKOsywg I6tw== X-Gm-Message-State: AODbwcCg0/K2Zp2JMVISADh279a4GfzuA92UqYCb6rfxrreNY98N07Y9 d7F9pq4dCy90ptgsnDNFVg== X-Received: by 10.223.138.250 with SMTP id z55mr2439417wrz.50.1495107244712; Thu, 18 May 2017 04:34:04 -0700 (PDT) Received: from [192.168.0.102] ([37.189.166.198]) by smtp.gmail.com with ESMTPSA id x17sm4405584wrd.63.2017.05.18.04.34.03 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Thu, 18 May 2017 04:34:04 -0700 (PDT) Subject: Re: [RFC 2/7] Add unit test to builtin tdesc generated by xml To: Yao Qi References: <1494518105-15412-1-git-send-email-yao.qi@linaro.org> <1494518105-15412-3-git-send-email-yao.qi@linaro.org> <692db623-3694-b809-4075-293c0d70cf5e@redhat.com> <86bmqq8p8h.fsf@gmail.com> Cc: gdb-patches@sourceware.org From: Pedro Alves Message-ID: <2590c996-7d14-07d5-9b0a-21ebaf9dc10b@redhat.com> Date: Thu, 18 May 2017 11:34:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.4.0 MIME-Version: 1.0 In-Reply-To: <86bmqq8p8h.fsf@gmail.com> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit X-SW-Source: 2017-05/txt/msg00418.txt.bz2 On 05/18/2017 10:54 AM, Yao Qi wrote: > Pedro Alves writes: > >>> +{ >>> + std::string feature_dir (ldirname (__FILE__)); >>> + struct stat st; >> >> Ugh. Obviously this can't work if gdb is installed / copied elsewhere, >> remote host testing, etc. >> > > I thought about this, but I can't figure out one better than __FILE__. > What I want to do is to find srcdir, and open these xml files during > unit tests. Since it is a unit test, I expect gdb is executed in either > builddir/gdb or builddir/gdb/testsuite. I don't see a case that people > build gdb in one place, and run unit/self tests somewhere else. Not sure -- does it work in remote host testing scenarios? Recall that unit testing is invoked by the normal testsuite, from gdb.gdb/unittest.exp. An alternative would be to add a specific command to run these tests, which would take the path as argument, like "maint check xml-descriptions /path/to/features/", and then run that from gdb.gdb/unittest.exp. > >>> + >>> + /* Look for the features directory. If the directory of __FILE__ can't >>> + be found, __FILE__ is a file name with relative path. Guess that >>> + GDB is executed in testsuite directory like ../gdb, because I don't >>> + expect that GDB is invoked somewhere else and run self tests. */ >>> + if (stat (feature_dir.data (), &st) < 0) >>> + { >>> + feature_dir.insert (0, SLASH_STRING); >>> + feature_dir.insert (0, ".."); >>> + >>> + /* If still can't find the path, something is wrong. */ >>> + SELF_CHECK (stat (feature_dir.data (), &st) == 0); >> >> Do we want to flag this as an error / unit test failure? >> Maybe it should be a warning instead? >> > > We can skip this test if it can't find "features" directory in source, but > something wrong can be easily ignored if we do so. > >>> --- a/gdb/target-descriptions.h >>> +++ b/gdb/target-descriptions.h >>> @@ -162,6 +162,12 @@ enum gdb_osabi tdesc_osabi (const struct target_desc *); >>> int tdesc_compatible_p (const struct target_desc *, >>> const struct bfd_arch_info *); >>> >>> +/* Compare target descriptions TDESC1 and TDESC2, return true if they >>> + are identical. */ >>> + >>> +bool tdesc_equals (const struct target_desc *tdesc1, >>> + const struct target_desc *tdesc2); >> >> Any reason this and the other equals functions aren't operator== >> implementations? > > tdesc_reg, tdesc_type, tdesc_feature and target_desc should be > class-fied first, including adding proper ctor, dtor, etc. There's no such requirement in the language. > I thought it > must be a lot of work, so I don't do that. I can do that if it doesn't > take me much time. But you don't need to do that to implement operators. > >> It's not obvious since the comments say "identical", which would maybe >> suggest that >> there may be some property that is not being compared and thus this is not >> strict value equality, but then function name says "equals". > > I think "identical" implies "strict value equality". Yeah, I think I got it backwards (been a long time since I used a programming language that has such a distinction), but the point still stands -- "identical" and "equals" can mean slightly different things so I'd rather not mix them up to avoid confusion. > The information I > want to deliver is that this function compares all properties, return > true if they are exactly the same. Is it this better, > > /* Return true if target description TDESC1 and TDESC2 are equal. */ Sure, that makes the comment matches the function name, so it's fine. Say "target descriptions", plural, though. Thanks, Pedro Alves