Date: Sat, 14 Jan 2012 19:05:31 -0500
From: Austin Clements <amdragon@MIT.EDU>
To: Jameson Graef Rollins <jrollins@finestructure.net>
Subject: Re: [PATCH v2 2/3] lib: Add support for automatically excluding tags
	from queries
Message-ID: <20120115000531.GF1801@mit.edu>
References: <1326258173-21163-1-git-send-email-amdragon@mit.edu>
	<1326496024-14403-1-git-send-email-amdragon@mit.edu>
	<1326496024-14403-3-git-send-email-amdragon@mit.edu>
	<87zkdpsvln.fsf@servo.finestructure.net>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <87zkdpsvln.fsf@servo.finestructure.net>
User-Agent: Mutt/1.5.21 (2010-09-15)
Cc: notmuch@notmuchmail.org
Precedence: list

Quoth Jameson Graef Rollins on Jan 14 at  3:38 pm:
> It looks like something in this patch is causing the following build
> warning:
> 
> CXX -O2 lib/query.o
> lib/query.cc:26:8: warning: ‘_notmuch_query’ declared with greater visibility than the type of its field
> ‘_notmuch_query::exclude_terms’ [-Wattributes]
> 
> However, I can't quite figure out what's causing it.

The problem is that notmuch_query_t is a "visible" symbol because the
type is declared (though not defined) in lib/notmuch.h.  The actual
definition is tucked away in lib/query.cc, but GCC doesn't seem to
care.  I added a field of type notmuch_string_list_t to the struct's
definition, but notmuch_string_list_t is declared between the "hidden"
pragmas in lib/notmuch-private.h because the type is private to the
library.  This field with a hidden type in a visible type is what GCC
is complaining about.

I'm rather confused by the whole type visibility thing since type
symbols aren't linkable anyway.  However, while puzzling over how you
could possibly use hidden types if they can only be used in other
hidden types, I discovered that Carl had solved this exact problem
last May in d5523ead by adding a visibility("default") attribute to
the offending hidden type.

> > +void
> > +notmuch_query_add_tag_exclude (notmuch_query_t *query, const char *tag)
> > +{
> > +    char *term = talloc_asprintf (query, "%s%s", _find_prefix ("tag"), tag);
> > +    _notmuch_string_list_append (query->exclude_terms, term);
> > +}
> 
> This is really not an issue with this patch at all, and it should NOT
> prevent it from being applied, but this came up briefly on IRC and I'm
> curious, so I'll ask about it here.
> 
> Are terms ALWAYS lower cased?  If not, it seems to me it's possible to
> have an indexed term 'Kspam' that would get confused with the term
> 'spam' prefixed with the keyword prefix 'K' (which we use for tags).
> Maybe this degeneracy is broken by the query parser somehow (or maybe by
> the fact that tags are boolean terms?), but I wonder if it's not safer
> to use the built-in xapian prefix separator ':', ie:
> 
>   ... talloc_asprintf (query, "%s:%s", _find_prefix ("tag"), tag);
> 
> I guess fixing that globally would require a database rebuild...

We discussed this on IRC, but to summarize for the list, the tag
prefix is a single character, so Xapian's ':' rule doesn't apply.
There are several places where we *do* get this wrong and use a
multi-character term prefix with a term that may start with a capital
letter but they're all terms you can't search anyway and, unless I'm
mistaken, we're completely consistent about where we violate or do not
violate the ':' rule.

> Ok, that's totally just an aside, and should not be a blocker for this
> patch.
> 
> jamie.